-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
stringzilla: add version 3.8.4 #22708
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Wait for merged. |
This comment has been minimized.
This comment has been minimized.
if(stringzilla_VERSION VERSION_LESS 2.0) | ||
target_compile_definitions(${PROJECT_NAME} PRIVATE STRINGZILLA_LESS_2_0) | ||
target_compile_definitions(${PROJECT_NAME} PRIVATE STRINGZILLA_API=1) | ||
elseif(stringzilla_VERSION VERSION_LESS 3.0) | ||
target_compile_definitions(${PROJECT_NAME} PRIVATE STRINGZILLA_API=2) | ||
else() | ||
target_compile_definitions(${PROJECT_NAME} PRIVATE STRINGZILLA_API=3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put this variable directly in the CMakeToolchain, something like tc.preprocessor_definitions["STRINGZILLA_API"] = Version(self.dependencies["stringzilla"].ref.version).major
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ahajha
Thank you for your comment!
I fixed as you suggested.
Could you review it again?
This comment has been minimized.
This comment has been minimized.
haystack.end() - haystack.begin() == haystack.size(); // Or `rbegin`, `rend` | ||
haystack.find_first_of(" \w\t") == 4; // Or `find_last_of`, `find_first_not_of`, `find_last_not_of` | ||
haystack.starts_with(needle) == true; // Or `ends_with` | ||
haystack.remove_prefix(needle.size()); // Why is this operation in-place?! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, for consistency add return EXIT_SUCCESS;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@RubenRBS |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 17 (
Conan v2 pipeline ✔️
All green in build 18 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your patience while I circled back to this PR - The simplifications look great, thanks a lot! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Specify library name and version: stringzilla/*