Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update class.cpp to make code compatible with MSVC #299

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

andriish
Copy link
Contributor

@andriish andriish commented Apr 9, 2024

Update class.cpp to make code compatible with MSVC.

Should solve #298

Update class.cpp to make code compatible with MSVC
Copy link
Member

@lyskov lyskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... @andriish i do not think this code is doing what you intended (see https://en.cppreference.com/w/cpp/string/basic_string/rfind). Please instead call begins_with from https://github.com/RosettaCommons/binder/blob/master/source/util.cpp#L88C6-L88C17
Thanks,

@andriish
Copy link
Contributor Author

Hi @lyskov,
the code does what is expected:

$ root -l
root [0] std::string return_type="class XXX"
(std::string &) "class XXX"
root [1] if (return_type.rfind("class ", 0) == 0) return_type = return_type.substr(6);
root [2] return_type
(std::string &) "XXX"
root [3] 

I havent seen your begins_with, will use it.
But I would actually update begins_with.

Andrii

@andriish andriish requested a review from lyskov April 12, 2024 10:15
@andriish
Copy link
Contributor Author

@lyskov

@lyskov
Copy link
Member

lyskov commented Apr 12, 2024

$ root -l
root [0] std::string return_type="class XXX"
(std::string &) "class XXX"
root [1] if (return_type.rfind("class ", 0) == 0) return_type = return_type.substr(6);
root [2] return_type
(std::string &) "XXX"
root [3] 

-- oh, i see, so the pos argument is offset from the beginning of the string and not the end 🤦🏻‍♂️, - yeah, that will work too!
Btw, do you use some kind of interpreter to test this, which one?

I havent seen your begins_with, will use it. But I would actually update begins_with.

-- could you please elaborate? Ie do you expect rfind implementation to be more efficient or something?
Thanks,

Copy link
Member

@lyskov lyskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, - thank you @andriish !

@lyskov lyskov merged commit b9f309e into RosettaCommons:master Apr 12, 2024
17 checks passed
@lyskov
Copy link
Member

lyskov commented Apr 12, 2024

I havent seen your begins_with, will use it. But I would actually update begins_with.

@andriish could it be that you are referring to the old study that claims that for natural languages it yield better performance to always search from the end of the strings and not beginning (ie there is less number of strings that ends the same compare to number of strings that have begin with same sequence)?

@andriish
Copy link
Contributor Author

-- The interpreter is https://root.cern/
-- The "rfind" is just "oneliner for string::starts_with" implemented in C++11

@lyskov lyskov mentioned this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants