Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
moves types common to indirectly_readable and indirectly_writable tests into a header
LGTM with comments addressed. I marked blocking comments as such and non-blocking comments as such.
| libcxx/include/iterator | ||
|---|---|---|
| 1859–1874 | 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. |
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. | |
| 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? | |
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. | |
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.
| 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.) | |
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 |
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.
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.
What is being obfuscated?
Added. | |
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.
| 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. | |
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 | 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 | ||
| 47 | Non-blocking: I'm not sure we need to check this and void*, int.Same below. | |
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!
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 workOf 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.
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 | No, that will create a circular dependency. | |
| libcxx/include/__iterator/iterator_traits.h | ||
|---|---|---|
| 42 | How so? iterator_traits.h should be able to include concepts.h. | |
| libcxx/include/__iterator/iterator_traits.h | ||
|---|---|---|
| 42 | 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).
| libcxx/include/__iterator/iterator_traits.h | ||
|---|---|---|
| 42 | 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). | |
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 | 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 | ||
| 47 | I do want some rudimentary tests ensuring void* isn't indirectly writable. | |
| 59–66 | Need to delete these since they're accounted for in their types' respective iterator_concept_conformance.cpp files. | |
after (much) offline discussion, adds the legacy output iterator test as well as a few others
one final attempt at getting CI to pass before pushing to main and letting scheduled CI take care of it (with @ldionne's permission)
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?
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.
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.
@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.
Shouldn't these go in __iterator/concepts.h?