-
-
Notifications
You must be signed in to change notification settings - Fork 740
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
ICU-22876 C++ UnicodeSet/USet easy item iteration #3120
Conversation
1051257
to
5f2dcfc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Hi everyone:
|
@richgillam do you care whether the prefix of the new classes, which operate on USet but are in C++ and also created by UnicodeSet, is USetXyz or UnicodeSetXyz? For example,
Right now, I am leaning towards the shorter version which seems slightly more accurate. @aheninger opinion? |
I used Craig as a sounding board for the class prefix naming question. What tipped him over the top is that the new types are in uset.h, so he would expect them to start with "USet". I like that. Will rename. |
Renamed. This would also allow future additions of UnicodeSetXyz that don't go through the C API, if we ever wanted that. |
Yes, I like USet here, for the reasons you outline. |
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.
Wow, that's a lot of code to do something that seems like it should be fairly simple!
I'm not sure I completely followed everything, but it all seems to make sense. But I'll defer to the other reviewers, all of whom understand what's going on here better than I do.
I agree! |
I think I am all done. It might still make sense to review this one commit at a time. |
Dear reviewers, it would be lovely to get this reviewed, feedback addressed, approved, and merged... fairly quickly, to unblock @roubert's follow-up work with the header-only collator predicates. I would be happy to jump into a video meeting to talk things through. |
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.
Looks like all that's changed since the last time I reviewed this is documentation and tests. The new documentation and tests look good.
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!
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.
New documentation also looks good.
ea03e6b
to
471d50d
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
Modern C++ iteration over the code points, ranges, or strings, or all of the elements, in a C++ UnicodeSet or C USet.
API approved by ICU-TC on 2024sep05.
Minor change sep09 during implementation testing:
Changed CodePointRange::iterator::operator++() from void to returning iterator & (usual signature)
Sample real, working call sites:
Output:
Checklist