This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Fix enable_if condition of std::reverse_iterator::operator=
ClosedPublic

Authored by miyuki on Nov 8 2021, 9:55 AM.

Details

Summary

The template std::is_assignable<T, U> checks that T is assignable from
U. Hence, the order of operands in the instantiation of
std::is_assignable in the std::reverse_iterator::operator= condition
should be reversed.

This issue remained unnoticed because std::reverse_iterator has an
implicit conversion constructor. This patch adds a test to check that
the assignment operator is used directly, without any implicit
conversions. The patch also adds a similar test for
std::move_iterator.

Diff Detail

Event Timeline

miyuki requested review of this revision.Nov 8 2021, 9:55 AM
miyuki created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2021, 9:55 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
libcxx/include/__iterator/reverse_iterator.h
114

C++17 actually doesn't constrain this operator= at all
https://timsong-cpp.github.io/cppwp/n4659/reverse.iter.op=
but C++20 added constraints
https://cplusplus.github.io/LWG/issue3435
which @ldionne added (in all modes) in e4d3a993c2675f46cbe99acd1bf6e6d39d9c1aee.
I believe we should make this constraint apply only in C++20, and follow the Standard's subtly different spec: convertible_to, assignable_from instead of is_convertible, is_assignable.

Scope creep: Any appetite for adding operator<=> at the same time, so that we could actually verify that three_way_comparable_with<reverse_iterator<int*>, reverse_iterator<const int*>> will be true after this patch? (I believe the only actually missing puzzle piece is operator<=>.)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp
97

Very nice catch!
As always, I'm not thrilled about misusing inheritance like this. I see that inheriting to_iter : const_bidir_iter is a cheap way to get the right Big Five iterator typedefs for your type, but it doesn't actually make to_iter into a bidirectional iterator (i.e. std::bidirectional_iterator<to_iter> == false), because it leaves its operator++ with the wrong return type, and so on. Meanwhile, the inheritance makes overload resolution more confusing than it needs to be.
But, at the same time, I'm not vehemently convinced that something manual like https://godbolt.org/z/bqM5hMEd4 would really be any better. :)

I believe test_assign_convertible() can easily be made constexpr-friendly and shoved into tests().

ldionne requested changes to this revision.Nov 8 2021, 1:35 PM

Thanks for catching and fixing this -- how embarrassing! I checked and it looks like move_iterator doesn't have the same problem -- would you mind adding a similar test to it just to make sure we don't regress it?

libcxx/include/__iterator/reverse_iterator.h
114

I added it in all modes because it's a LWG issue, not only a paper. I think that is what we want to do, however you are right we can't implement it exactly per the spec if we backport it to before C++20 (because we don't have concepts then).

This revision now requires changes to proceed.Nov 8 2021, 1:35 PM
miyuki updated this revision to Diff 386583.Nov 11 2021, 10:33 AM
miyuki edited the summary of this revision. (Show Details)
  1. Updated std::reverse_iterator tests
  2. Added a test for std::move_iterator
miyuki marked an inline comment as done.Nov 11 2021, 10:39 AM
miyuki added inline comments.
libcxx/include/__iterator/reverse_iterator.h
114

Scope creep: Any appetite for adding operator<=>

Submitted as a separate patch: D113695

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp
40

I recommend eliminating this typedef. Right now there's a symmetry in the names of FromIter and ToIter but actually the former is not a class type.

73

Why is this not just move_ti = move_fi;?
What you've got won't necessarily work on non-libc++ implementations.
Orthogonally(?), if you want to check that this is really calling the "right" operator= instead of doing an implicit conversion followed by the "wrong" operator=, then you could make those two operators do different things; e.g.

ToIter(const FromIter& src) { assert(false); }
TEST_CONSTEXPR_CXX17 ToIter& operator=(const FromIter& src) { m_value = 1; return *this; }
TEST_CONSTEXPR_CXX17 ToIter& operator=(FromIter&& src) { m_value = 1; return *this; }
[...]
move_ti = move_fi;
assert(move_ti.m_value == 1);
move_ti = std::move(move_fi);
assert(move_ti.m_value == 2);

Ditto in the reverse_iterator test below.

miyuki updated this revision to Diff 386782.Nov 12 2021, 3:40 AM

Updated tests

miyuki marked 2 inline comments as done.Nov 12 2021, 3:50 AM
miyuki added inline comments.
libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp
73

Why is this not just move_ti = move_fi;

assert(false); would not work in constexpr context, so I used an explicit call of operator= as a workaround (it would fail at compile time without the fix in reverse_iterator.h). I found a simpler workaround for assert(false), so now I can use move_ti = move_fi;.

Orthogonally(?), if you want to check that this is really calling the "right" operator=

An assertion in the conversion constructor should be enough to test the fix.

miyuki marked an inline comment as done.Nov 12 2021, 6:20 AM

The AIX failures seem to be unrelated.

ldionne accepted this revision.Nov 12 2021, 11:22 AM
ldionne added a subscriber: daltenty.

This LGTM. The CI failures is indeed unrelated to this change, so this should be safe to check in.

Please give a bit of time to @Quuxplusone to say if he's still got comments on this, but otherwise let's go ahead. Thanks a lot for fixing this.

@daltenty Can you please take a look at the AIX bots, the seem to have started failing over the night: https://buildkite.com/llvm-project/libcxx-ci/builds/6599#a2b5b4eb-95e7-44bc-b7e1-549367a4be52

This revision is now accepted and ready to land.Nov 12 2021, 11:22 AM

@daltenty Can you please take a look at the AIX bots, the seem to have started failing over the night: https://buildkite.com/llvm-project/libcxx-ci/builds/6599#a2b5b4eb-95e7-44bc-b7e1-549367a4be52

@ldionne Taking a look now, suspect some host machine updates changes something locale wise.

Quuxplusone accepted this revision.Nov 12 2021, 12:28 PM

LGTM mod a few last comments on the test.

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp
50–53

I believe a better fix would be to remove TEST_CONSTEXPR_CXX17 from this function:

ToIter(char *) { assert(false); }

This function should never be called at all. You could even leave off the body if you want:

ToIter(char *);

(But you can't =delete it, because the library is checking that is_convertible_v<char*, ToIter>.)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp
43–48

Ditto here: just ToIter(char *); should suffice.

miyuki updated this revision to Diff 387193.Nov 15 2021, 3:25 AM

Rebased and addressed Arthur's comment.

miyuki marked 2 inline comments as done.Nov 15 2021, 3:26 AM