-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Optimizations for contiguous iterators #1433
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
Merged
StephanTLavavej
merged 26 commits into
microsoft:main
from
AdamBucior:contiguous-iterator-optimizations
Mar 23, 2021
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
75474b6
Optimizations for contiguous iterators
AdamBucior b2d7591
Projections need to be same as identity
AdamBucior 30da25d
clang-format
AdamBucior b1b6a83
clang-format please cooperate
AdamBucior 6450aaf
Wrong iterators
AdamBucior e6058d4
remove_const
AdamBucior 2952906
Fix find
AdamBucior cadd6cc
Merge branch 'master' into contiguous-iterator-optimizations
AdamBucior 876358d
_Memcmp_ranges
AdamBucior 3fbfec8
_Memchr_in_find_is_safe
AdamBucior 094b2b2
Simplify _Within_limits
AdamBucior 1a1cd0a
Merge branch 'master' into contiguous-iterator-optimizations
AdamBucior 0d417ea
remove _HAS_IF_CONSTEXPR
AdamBucior 8322e97
Merge branch 'master' into contiguous-iterator-optimizations
AdamBucior acd8c3f
Apply suggestions from code review
AdamBucior 23bf922
Code review
AdamBucior 9e83f10
clang-format
AdamBucior f706346
static_cast
AdamBucior ed044f3
Merge branch 'master' into contiguous-iterator-optimizations
AdamBucior 65128e6
Merge branch 'master' into contiguous-iterator-optimizations
AdamBucior 3e91462
Revert "Merge branch 'master' into contiguous-iterator-optimizations"
AdamBucior 3cdd6f8
Merge branch 'master' into contiguous-iterator-optimizations
AdamBucior fbc0c56
Merge branch 'main' into contiguous-iterator-optimizations
StephanTLavavej a1b8227
Move _Find_unchecked down.
StephanTLavavej ccf9f49
Code review feedback.
StephanTLavavej 7263e21
Refine the condition for calling _Memcmp_ranges.
StephanTLavavej File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Nitpick: i believe
_Numis a bit too generic. Maybe use_Lenor_SizeinsteadThere 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.
_Numis already used in the originalstd::lexicographical_compareandstd::lexicographical_compare_three_way(and in lot's of other places) and after all it clearly describes what it is - the number of elements.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.
I agree with @miscco's point (
_Num\d*is relatively uncommon, and a bit more generic - one could speak of a number of matching elements possibly with gaps, while len/size is more specific), but I also agree with @AdamBucior's point - this is consistent with existing usage and seems to be clear so there is no risk of confusion.The thing that would tip the scales for me is if we had multiple numbers of stuff involved - then it would be good to give distinct names to each. That isn't the case here, where we just have 1/2 mirroring the ranges named 1/2. Thus I think that the code is fine as-is.
Thanks for bringing this up! Clear naming is indeed important 😸