Page MenuHomePhabricator

[libc++] Make `_IterOps::__iter_move` more similar to `std::ranges::iter_move`.
ClosedPublic

Authored by var-const on Jul 25 2022, 8:12 PM.

Details

Summary

Avoid relying on iterator_traits and instead deduce the return type of
dereferencing the iterator. Additionally, add a static check to reject
iterators with incorrect iterator_traits at compile time.

Diff Detail

Event Timeline

var-const created this revision.Jul 25 2022, 8:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 8:12 PM
var-const requested review of this revision.Jul 25 2022, 8:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 8:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser added a subscriber: jloser.Jul 25 2022, 8:20 PM

I don't personally love changing the code here for a regression that caught user code UB, but I understand.

In any case, can we add a test for the "invalid proxy iterator" case to bind this support to avoid future regressions in this area, please?

jgorbe added a subscriber: jgorbe.Jul 25 2022, 8:32 PM

Are you absolutely certain that this won't break anything else? I'm definitely not, so I'd say rather break invalid code than valid code.

ldionne requested changes to this revision.Jul 26 2022, 11:42 AM
ldionne added a subscriber: ldionne.

I like this new approach regardless of the issues raised in D129823 because it is closer to decltype(auto), and that's what we're trying to achieve at the end of the day.

However, the truth is that any iterator that lies about its iterator_traits::reference is a potential issue just waiting to explode, and since we can easily diagnose it, I think we should. That's why I've suggested adding a static_assert as a QOI thing.

Is it going to break some code? Probably, but that will actually point out potential bugs in people's code, which is a nice property to have.

libcxx/include/__algorithm/iterator_operations.h
68

This way, we'll match the definition of iter_reference_t.

71

IMO that's easier to read, since I don't have to substitute __deref_t in my head and work through the additional declval<T>() layer.

77–80
84–85

(suggested message, feel free to change)

The full link is http://eel.is/c++draft/input.iterators#tab:inputiterator.

Also, please add a libc++ specific test to ensure that we do trigger the static_assert at least when we call std::sort with a mis-behaving iterator.

This revision now requires changes to proceed.Jul 26 2022, 11:42 AM
var-const updated this revision to Diff 447931.Jul 26 2022, 9:37 PM
var-const marked 4 inline comments as done.

Address feedback.

Are you absolutely certain that this won't break anything else? I'm definitely not, so I'd say rather break invalid code than valid code.

I'm not absolutely certain, but the same can be said of any change of non-trivial complexity. Even completely ignoring the MySQL issue, this change is arguably an improvement because it brings this implementation closer to how the actual C++20 iter_move is specified, and at the end of the day we are trying to emulate that. I believe we would have used decltype here from the start, but we had the (incorrect) assumption that it's not supported in C++03. If you have a specific concern, I'm of course happy to address it.

libcxx/include/__algorithm/iterator_operations.h
84–85

Done (with a slight tweak to the wording).

I created a helper function and added this assertion to both specializations of __iter_move.

var-const retitled this revision from [libc++] Fix `_IterOps::__iter_move` returning a dangling reference for non-conforming iterators. to [libc++] Make `_IterOps::__iter_move` more similar to `std::ranges::iter_move`..Jul 26 2022, 9:39 PM
var-const edited the summary of this revision. (Show Details)

For future context, the issue of incorrect iterator_traits came up from a breakage in MySQL; more context here: https://reviews.llvm.org/D129823#3677969.

For future context, the issue of incorrect iterator_traits came up from a breakage in MySQL; more context here: https://reviews.llvm.org/D129823#3677969.

ldionne accepted this revision.Jul 27 2022, 5:00 AM

LGTM pending CI.

This revision is now accepted and ready to land.Jul 27 2022, 5:00 AM
huixie90 accepted this revision.Jul 27 2022, 12:25 PM
huixie90 added a subscriber: huixie90.
huixie90 added inline comments.
libcxx/include/__algorithm/iterator_operations.h
76–77

nit: i think the comments no long apply anymore. The return type also has sfinae logic in it

93

extra nit: perhaps we don't need to forward it, because the __deref_t and _move_t are defined in terms of lvalue of _Iter

var-const updated this revision to Diff 448187.Jul 27 2022, 3:33 PM

Fix the CI (a constexpr function can only return void starting from C++14).

var-const updated this revision to Diff 448192.Jul 27 2022, 3:47 PM

Address feedback.

var-const marked 2 inline comments as done.Jul 27 2022, 3:54 PM
var-const added inline comments.
libcxx/include/__algorithm/iterator_operations.h
76–77

Rephrased the comment to make it clearer that we can't use decltype(auto) or auto as the return type, regardless of the SFINAE.

93

Discussed offline -- this makes it closer to how iter_reference_t and iter_move are defined in C++20, and there doesn't seem to be an observable difference.