[libcxx-commits] [PATCH] D108855: [libcxx] contiguous iterator concept: don't require pointer or complete element types

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:106-107
     typedef std::ptrdiff_t                  difference_type;
     typedef int*                            pointer;
     typedef short&                          reference;
     typedef wrong_iter_reference_t          self;
----------------
This type is misnamed or misimplemented. It's called `wrong_iter_reference_t`, but actually its `reference` type is correct; it's the `pointer` type that is "wrong."
I suggest changing it to have
```
    typedef short     element_type;
    typedef short*    pointer;
    typedef int*      reference;
```
so that the name can remain the same. However, please check whether there's already another test case in this file for the wrong-reference-type case.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:133
 static_assert(std::random_access_iterator<wrong_iter_reference_t>);
-static_assert(!std::contiguous_iterator<wrong_iter_reference_t>);
 
----------------
cjdb wrote:
> jloser wrote:
> > cjdb wrote:
> > > This test needs to remain. 
> > I made this test pass by adding an `element_type` type alias to avoid the hard error in evaluating `std::to_address`.  Does that work for you?
> Thank you! 
LGTM too, mod my comment on line 107. I checked GCC and MSVC, and neither of them is SFINAE-friendly in this case, so we don't need to be SFINAE-friendly either.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:167
 static_assert(std::random_access_iterator<no_element_type>);
-static_assert(!std::contiguous_iterator<no_element_type>);
 
----------------
cjdb wrote:
> jloser wrote:
> > cjdb wrote:
> > > As does this one. 
> > I think this test is ill-formed due to the lack of an `element_type` type alias. This exposes a hard error when instantiating `std::to_address` partial specialization which is intended (and correct) behavior I believe.
> Ack, but the test is still useful. Please move this to a verification test so we can keep it. 
Agreed that the test is ill-formed: I checked GCC and MSVC, and neither of them is SFINAE-friendly in this case, so we don't need to be SFINAE-friendly either.
I don't see any value in checking the exact wording of Clang's error message here. I wouldn't move this test, just delete it.

Delete lines 136–168.  (This is a test file for `contiguous_iterator`, so there's no point in keeping any of the test case's code. In isolation, we don't care that this type happens to be a `random_access_iterator`.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108855/new/

https://reviews.llvm.org/D108855