-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libc++] Merge the private iterator_traits aliases with their ranges versions #162661
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
Open
philnik777
wants to merge
1
commit into
llvm:main
Choose a base branch
from
philnik777:merge_iter_traits_aliases
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
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
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
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
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.
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.
Why is it okay to make this (general) change? I understand that we are probably just accepting more code than previously, but it is possible for conforming C++20 code to break when we do this?
A few questions:
std::iter_value_t<It>
andstd::iterator_traits<It>::value_type
?iterator_traits
, or at least did historically) would break if we start usingstd::iter_value_t<It>
instead?Also CC @frederick-vs-ja since I think I remember having that discussion in a previous patch.
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.
Your proofs:
For
iter_reference_t
, https://eel.is/c++draft/iterator.cpp17#input.iterators-2 says that*a
must beiterator_traits::reference
for a C++17 iterator. Henceiterator_traits::reference
anddecltype(*a) == iter_reference_t<It>
must be the same. ✅For
iter_difference_t
, we have two cases. Ifiterator_traits<It>
is specialized, incrementable.traits says thatiter_difference_t<It>
isiterator_traits<It>::difference_type
. If not specialized, theniter_difference_t<It>
isincrementable_traits<It>::difference_type
, whatever that is. However, in C++20,iterator_traits
also changed to be more C++20 friendly (I guess):if
It::difference_type
andIt::value_type
andIt::reference
(etc..) do exist, theniterator_traits<It>::difference_type
isIt::difference_type
. However, in that case,incrementable_traits<It>::difference_type
is alsoIt::difference_type
, unless it is specialized explicitly. And it's legal to specialize it explicitly. This means that technically, if someone specializesincrementable_traits<It>
to give it adifference_type
that doesn't matchiterator_traits<It>::difference_type
, this patch will produce a difference. It's unclear whetheris intended to override the normal requirement that specializations must be equivalent to the base template. This is probably material for a quick LWG issue. ❌
if
It::difference_type
or any of its friends do not exist,iterator_traits<It>::difference_type
is defined to beincrementable_traits<It>::difference_type
, so the equivalence holds. ✅For
iter_value_t
,iter_value_t<It>
isiterator_traits<It>::value_type
ifiterator_traits<It>
is specialized. Otherwise, it isindirectly_readable_traits<It>::value_type
, whatever that is. However,It::difference_type
andIt::value_type
andIt::reference
(etc..) do exist, theniterator_traits<It>::value_type
isI::value_type
. And in that caseindirectly_readable_traits<It>::value_type
is a rather complicated thing. It seems that ifIt::element_type
andIt::value_type
disagree,indirectly_readable_traits<It>::value_type
would not be defined whereasiterator_traits<It>::value_type
would be. There's also the specialization problem above. And there's the issue of legacy output iterators whereIt::value_type == void
, as explained in this note. ❌It::value_type
or any of its friends do not exist,iterator_traits<It>::value_type
is defined toindirectly_readable_traits<It>::value_type
. ✅So, in summary, I think we have the following issues:
iterator_traits<It>::difference_type
may beIt::difference_type
butincrementable_traits<It>
may be something else via a specializationiterator_traits<It>::value_type
may beIt::value_type
butindirectly_readable_traits<It>::value_type
does not existiterator_traits<It>::value_type
may beIt::value_type
butindirectly_readable_traits<It>
may be specializediterator_traits<It>::value_type
may beIt::value_type == void
butindirectly_readable_traits<It>
does not existThere 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.
Now, the question is whether we actually care about these differences.
I think we don't care about the specialization stuff. We should get those clarified with a LWG issue, but I have no sympathy for someone who has different
It::difference_type
andincrementable_traits<It>::difference_type
through a specialization. Same forIt::value_type
.However, the case of legacy output iterators could be a real problem. However, we basically never use
iterator_traits<It>::value_type
for an output iterator since it may bevoid
. So in practice this might not matter.I'll wait for @frederick-vs-ja to chime in, but so far I think I'd be comfortable with making this change. However, this would have to be dry-run on large code bases (we can ping the Google folks and we'll also do an internal build when we're ready), to shake off any unintended consequences.
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 submitted LWG4080. Perhaps we should add some discussions to it.
I think overriding was intended. The standard library contains such design for
common_iterator
, although this looks quite questionable to me. If overriding is considered harmful (as it can create inconsistent value types), I think it makes more sense to disallow some user-provided specializations (and slightly fixcommon_iterator
).I'm more worried about this. There can be C++17 iterators with evil
element_type
(I initially reported this in microsoft/STL#4436) which make them not C++20 iterators. If we use__iter_value_t
like MSVC STL's current approach, some code using legacy algorithms can be accepted in C++17 but rejected in C++20.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'm really not that worried about a
value_type
/element_type
mismatch. That rather seems to me like something worth diagnosing, since it's almost certainly user error. I'm not sure it's actually specified anywhere, but the intention thatremove_reference_t<decltype(*iter)>
iselement_type
seems rather clear to me.