This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][iterator] adds `std::indirectly_readable` and `std::indirectly_writable`
ClosedPublic

Authored by cjdb on Apr 7 2021, 3:53 PM.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 7 2021, 3:53 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2021, 3:53 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 335961.Apr 7 2021, 4:58 PM

moves types common to indirectly_readable and indirectly_writable tests into a header

cjdb updated this revision to Diff 336698.Apr 11 2021, 2:38 PM

adds synopsis, rebases to activate CI

ldionne accepted this revision.Apr 14 2021, 2:04 PM

LGTM with comments addressed. I marked blocking comments as such and non-blocking comments as such.

libcxx/include/iterator
2521–2536

This LGTM.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
121

You could use shorter names and instead use a comment to explain what this specific sub-test does. This way, you wouldn't have to carry around a name with almost 120 characters, which is kind of heavy on the eye and obscures the intent of the test.

This one is blocking.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/read_write.h
1 ↗(On Diff #336698)

Does it make sense to move this to test/support instead? Non blocking.

This revision is now accepted and ready to land.Apr 14 2021, 2:04 PM
zoecarver requested changes to this revision.Apr 14 2021, 2:17 PM

I have at least these few small comments, so please hold off on committing.

I'm not done reviewing yet, but I saw @ldionne approved it, so I wanted to get these in before you land it.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
39

I suppose I don't feel strongly, but are you using this elsewhere? Otherwise, maybe it makes sense to line the header; it's only 30 lines. WDYT?

58

Might as well put this in the header too, no? I think it makes sense to have all these types live together.

zoecarver added inline comments.Apr 14 2021, 2:17 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
94

Nit: Maybe missing_value_type is more accurate?

99

This type looks unused.

115

What's the difference between this test and indirection_mismatch? Maybe value_type should be iter_ref1 here? Because this should still fail, right?

129

This looks very similar to iter_move_mismatch. What's the difference?

This revision now requires changes to proceed.Apr 14 2021, 2:17 PM

Overall, I think this patch is great. There are a lot of tests, but they're all super simple and easily verifiable. Other than the few very small comments I made inline, this LGTM.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
2

I really like all the tests in here, thanks!

146

A bit shorter: different_reference_types_with_common_reference?

309

Only one template param.

cjdb updated this revision to Diff 337808.Apr 15 2021, 9:54 AM

rebases to activate CI

ldionne requested changes to this revision.Apr 15 2021, 12:24 PM

Actually, while looking at a different review, I stumbled upon this note: http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-1

Some legacy output iterators define a nested type named value_­type that is an alias for void. These types are not indirectly_­readable and have no associated value types.

I don't think we have a test for that here, so we should add one if we don't. Then there's also this note: http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-2, but I see we do have a test for that (great).

Requesting changes so this shows up correctly in the queue.

This revision now requires changes to proceed.Apr 15 2021, 12:24 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
309

I can't follow all the template metaprogramming on lines 152-175, and lines 184-283 are obviously overkill (all iterator types are indirectly readable, by definition; we don't need a million header dependencies to prove that). The use of check_indirectly_readable<T>() in place of std::indirectly_readable<T> is also a bit obfuscatory; I would prefer to see the concept tested directly.

One test I'd like to see added is

struct NoConst { int& operator*(); };
static_assert(!std::indirectly_readable<NoConst>);

This is the sort of thing that (A) may come up in real code, and (B) may break under maintenance, if someone changes (const _In __in) to (_In __in) on line 2507. (requires-expression parameters are frequently treated the same as function parameters, and one would never const-qualify a function parameter; but here, the const keyword on the requires-parameter is actually the key to conformance. And we have no test for it.)

cjdb updated this revision to Diff 338409.Apr 18 2021, 6:54 PM
cjdb marked 2 inline comments as done.

responds to feedback

cjdb added a comment.Apr 18 2021, 6:55 PM

Actually, while looking at a different review, I stumbled upon this note: http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-1

Some legacy output iterators define a nested type named value_­type that is an alias for void. These types are not indirectly_­readable and have no associated value types.

I don't think we have a test for that here, so we should add one if we don't. Then there's also this note: http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-2, but I see we do have a test for that (great).

Requesting changes so this shows up correctly in the queue.

Are you sure? test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:179-87 already check for a void value_type/element_type. It doesn't really matter whether or not the type is an iterator.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
121

How about now?

129

It's a distinct type (one per test is necessary since common_reference needs to resolve as a different type).

309

I can't follow all the template metaprogramming on lines 152-175

common_reference_t<iter_ref4, iter_rvalue_ref> is iter_rvalue_ref and common_reference<iter_ref4 const&, iter_rvalue_ref&&>::type doesn't exist.

lines 184-283 are obviously overkill (all iterator types are indirectly readable, by definition; we don't need a million header dependencies to prove that)

This ensures conformance, which is a non-obvious thing to confirm. A full review of the tests would have revealed that not "all" iterators are indirectly readable, by definition. I'll leave it as an exercise for you to find those that don't satisfy the concept.

However, at your insistence, I've removed them from this file.

The use of check_indirectly_readable<T>() in place of std::indirectly_readable<T> is also a bit obfuscatory; I would prefer to see the concept tested directly.

What is being obfuscated?

One test I'd like to see added is

struct NoConst { int& operator*(); };
static_assert(!std::indirectly_readable<NoConst>);

This is the sort of thing that (A) may come up in real code, and (B) may break under maintenance, if someone changes (const _In __in) to (_In __in) on line 2507. (requires-expression parameters are frequently treated the same as function parameters, and one would never const-qualify a function parameter; but here, the const keyword on the requires-parameter is actually the key to conformance. And we have no test for it.)

Added.

cjdb updated this revision to Diff 338905.Apr 20 2021, 9:36 AM
cjdb marked 3 inline comments as done.

rebases so @zoecarver can continue working

The addition of indirectly_writable is accidental (probably a rebase gone wrong) and will be fixed in the next arc diff.
It shouldn't affect @zoecarver's work at all.

cjdb added a comment.Apr 20 2021, 9:45 AM

rebases so @zoecarver can continue working

The addition of indirectly_writable is accidental (probably a rebase gone wrong) and will be fixed in the next arc diff.
It shouldn't affect @zoecarver's work at all.

In hindsight, I think it might be easier to abandon D100078 since it's magically transported into D100073.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
58

These types are exclusively for indirectly_readable, so I'd prefer to keep them here.

zoecarver accepted this revision as: zoecarver.Apr 20 2021, 10:05 AM

This still mostly LGTM sans a few small comments.

Do you want to just merge this and indirectly writable into one patch? I'd be fine with that FWIW and it might make it easier to keep track of on your end.

Also, for what it's worth: I don't have a preference as to how you test the standard types. I think the current implementation (have each test be its own file) is nice, but also looks like a lot of work to write. So in the future, do whatever you'd prefer/is easier (imho).

libcxx/include/__iterator/iterator_traits.h
42 ↗(On Diff #338905)

Shouldn't these go in __iterator/concepts.h?

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
89

Non-blocking, nit: Apparently we're suppose to do this without opening the std namespace.

105

This name is long and hard to grok. Let's call it something like unrelated_ref.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.writable/indirectly_writable.compile.pass.cpp
46 ↗(On Diff #338905)

Non-blocking: I'm not sure we need to check this and void*, int.Same below.

zoecarver requested changes to this revision.EditedApr 20 2021, 10:12 AM

Actually, I'm still not seeing a test where value_type = void. Please add that per Louis comment / http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-1

Edit: I do however see {shared,unique}_ptr<void>, so that's good!

This revision now requires changes to proceed.Apr 20 2021, 10:12 AM

Re @zoecarver's comment about value_type = void: I observe that std::ostream_iterator<int> is such a legacy output iterator today. However, please add a test that uses a simpler type, because I don't trust ostream_iterator to continue providing a value_type member for the next decade.

The Standard's wording is sadly very opaque and confusing and scattered all over [iterators]; I'm not sure how to achieve a type with value_type = void that doesn't fall foul of many other constraints. Maybe something like this:

struct ValueTypeVoid {
    using value_type = void;
    using iterator_category = std::output_iterator_tag;
    using pointer = void*;
    using reference = int&;
    using difference_type = int;
};
static_assert(std::same_as<std::iter_value_t<ValueTypeVoid>, void>);
static_assert(!std::indirectly_readable<ValueTypeVoid>);
static_assert(!requires { typename std::iter_reference_t<ValueTypeVoid> });  // or whatever one has to type to make this work

Of course anything with value_type = void can't be indirectly_readable, because the definition of indirectly_readable requires forming iter_value_t<In>& as part of its common-reference checks.

cjdb added a comment.Apr 20 2021, 10:47 AM

Actually, I'm still not seeing a test where value_type = void. Please add that per Louis comment / http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-1

Edit: I do however see {shared,unique}_ptr<void>, so that's good!

I don't think this isn't a property of std::indirectly_readable: it's a property of std::indirectly_readable_traits. Please check lines 179-87 in indirectly_readable_traits.compile.pass.cpp for confirmation.

libcxx/include/__iterator/iterator_traits.h
42 ↗(On Diff #338905)

No, that will create a circular dependency.

zoecarver added inline comments.Apr 20 2021, 10:50 AM
libcxx/include/__iterator/iterator_traits.h
42 ↗(On Diff #338905)

How so? iterator_traits.h should be able to include concepts.h.

cjdb added inline comments.Apr 20 2021, 10:59 AM
libcxx/include/__iterator/iterator_traits.h
42 ↗(On Diff #338905)

concepts.h needs to include iterator_traits.h because it needs the iterator tag types. I could alternatively put them in their own small header, but something needs to be moved around.

I don't think this isn't a property of std::indirectly_readable: it's a property of std::indirectly_readable_traits. Please check lines 179-87 in indirectly_readable_traits.compile.pass.cpp for confirmation.

Yeah, I'm glad you have that test. But the standard specifically says indirectly_readable and links to the concept. So, I don't think it would hurt to add something like Arthur commented (even though anything with a void type will be rejected during the second check for iter_reference_t).

zoecarver added inline comments.Apr 20 2021, 11:43 AM
libcxx/include/__iterator/iterator_traits.h
42 ↗(On Diff #338905)

So, no one else needs __dereferenceable and friends? Fair enough. I'm fine with this in that case. I'd also support moving the tags into their own header, but I don't feel strongly one way or another (we can always move stuff around after the fact).

cjdb marked 4 inline comments as done.Apr 20 2021, 2:23 PM

I don't think this isn't a property of std::indirectly_readable: it's a property of std::indirectly_readable_traits. Please check lines 179-87 in indirectly_readable_traits.compile.pass.cpp for confirmation.

Yeah, I'm glad you have that test. But the standard specifically says indirectly_readable and links to the concept. So, I don't think it would hurt to add something like Arthur commented (even though anything with a void type will be rejected during the second check for iter_reference_t).

I really don't think this test is necessary. The note is offering commentary on why legacy iterators with a void value type have an empty indirectly_readable_traits. That transitively means the iterator won't be indirectly_readable.

libcxx/include/__iterator/iterator_traits.h
42 ↗(On Diff #338905)

Hmm good point. I'll make a tags header instead.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
94

Not sure what this is being applied to (seems to have been displaced).

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.writable/indirectly_writable.compile.pass.cpp
46 ↗(On Diff #338905)

I do want some rudimentary tests ensuring void* isn't indirectly writable.

58–65 ↗(On Diff #338905)

Need to delete these since they're accounted for in their types' respective iterator_concept_conformance.cpp files.

cjdb updated this revision to Diff 339025.Apr 20 2021, 4:03 PM
cjdb retitled this revision from [libcxx] adds `std::indirectly_readable` to <iterator> to [libcxx][iterator] adds `std::indirectly_readable` and `std::indirectly_writable`.

rebases to activate CI

cjdb updated this revision to Diff 339087.Apr 20 2021, 8:28 PM

removes circular dependency

cjdb updated this revision to Diff 339270.Apr 21 2021, 9:22 AM

after (much) offline discussion, adds the legacy output iterator test as well as a few others

ldionne accepted this revision.Apr 21 2021, 9:24 AM
cjdb updated this revision to Diff 339274.Apr 21 2021, 9:36 AM

rebases to activate CI

cjdb updated this revision to Diff 339280.Apr 21 2021, 9:42 AM

one final attempt at getting CI to pass before pushing to main and letting scheduled CI take care of it (with @ldionne's permission)

This revision was not accepted when it landed; it landed in state Needs Review.Apr 21 2021, 10:15 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

Hi,

With this patch I get errors in all the new testcases. I get

error: no member named 'indirectly_readable' in namespace 'std'

and

error: no member named 'indirectly_writable' in namespace 'std'

I noticed that if I explicitly add

#include <__iterator/concepts.h>

it seems to find indirectly_readable and indirectly_writable and the testcase I tried in passed.
I've looked at iterator and see it includes <concepts>, but I can't find anyone actually including <__iterator/concepts.h>?

How is this supposed to work?

Hi,

With this patch I get errors in all the new testcases. I get

error: no member named 'indirectly_readable' in namespace 'std'

and

error: no member named 'indirectly_writable' in namespace 'std'

I'm also seeing this. I also noticed that the last completed pre-merge check build of this failed to even build on Windows: https://buildkite.com/llvm-project/libcxx-ci/builds/2685#3708ce97-e66e-451f-99c0-a5ad71dfcc0e

The pushed version does seem to build, but fails a couple dozen tests.

davezarzycki added a subscriber: davezarzycki.EditedApr 22 2021, 6:52 AM

I'm the third person to notice the same problems, but this time on Fedora 34 (x86-64) so I reverted the change.

@ldionne -- if you need help debugging this, please let me know.

I'm looking into this now. Thanks for the heads up.

@ldionne @cjdb This is still failing in the Windows CI, see e.g. https://buildkite.com/llvm-project/libcxx-ci/builds/2747#766d601e-b153-4c1f-a649-dde27e2d6276 for the latest scheduled CI run on the main branch:

In file included from C:\ws\w16c2-2\llvm-project\libcxx-ci\libcxx\src\optional.cpp:9:
In file included from C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\optional:154:
In file included from C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\functional:514:
In file included from C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\memory:675:
In file included from C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\iterator:441:
C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\__iterator/concepts.h(40,13): error: expected expression
    { *__in } -> same_as<iter_reference_t<_In> >;
            ^
C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\__iterator/concepts.h(41,7): error: no matching function for call to object of type 'const std::__1::ranges::__iter_move::__fn'
    { ranges::iter_move(__in) } -> same_as<iter_rvalue_reference_t<_In> >;
      ^~~~~~~~~~~~~~~~~
C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\__iterator/iter_move.h(48,42): note: candidate function template not viable: requires single argument '__i', but no arguments were provided
  [[nodiscard]] constexpr decltype(auto) operator()(_Ip&& __i) const
                                         ^
C:/ws/w16c2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1\__iterator/iter_move.h(61,42): note: candidate function template not viable: requires single argument '__i', but no arguments were provided
  [[nodiscard]] constexpr decltype(auto) operator()(_Ip&& __i) const
                                         ^
2 errors generated.

This doesn't seem to happen in MinGW configurations, only when impersonating MSVC. The CI configuration still runs with Clang 11.0, but I just tested with Clang 12.0 and I get the exact same issue there too.