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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp:15
+
+#include <compare>
+#include <cstddef>
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > Minor nit, I would also include `<concepts>` since that's the header under test. Even when `<iterator>` is required to do so, especially since `<iterator>` is required to also include `<compare>`.
> > > Yes. libc++ test style is to start with the header under test (in this case, `<concepts>`); then follow it with the other necessary standard headers (in a-z order); and then (if needed) "test_macros.h" etc.
> > It's `<iterator>` that's under test. There's nothing from `<concepts>` that's directly used, so it can't be the one under test.
> Ah, right, `std::contiguous_iterator` is defined in `<iterator>`, not `<concepts>`. OK.
You're right.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp:57-58
+    // when used with a type missing an `element_type` type alias.
+    (void) std::contiguous_iterator<no_element_type>; // expected-note at __iterator/concepts.h:* 1 {{to_address}}
+    // expected-error at __memory/pointer_traits.h:* 1 {{implicit instantiation of undefined template 'std::__pointer_traits_element_type<no_element_type, false>'}}
+
----------------
cjdb wrote:
> jloser wrote:
> > Quuxplusone wrote:
> > > I'm still not clear on when we should name things `.fail.cpp` versus `.verify.cpp`. I would have expected this to be a `.verify.cpp`, but I'm not sure why.
> > > In either case, this is certainly testing libcxx-specific behavior, but my understanding is that it's still OK to put it in libcxx/test/std/ because it's not a `.pass.cpp`? Again I'm not 100% sure.
> > > I'd use `expected-error@*:*` instead of `expected-error at __memory/pointer_traits.h:*`. The exact name of the header file is a detail in this case (and also it'll make the line shorter). Also, you can remove the `1 ` from both lines.
> > > 
> > > Also also, since this test is not expected to compile, it doesn't need a `main`. I'd rather see line 52 say `void test()`, and then eliminate line 60.
> > I wasn't sure on the naming either -- `.fail.cpp` versus `.verify.cpp`. I looked around `libcxx` tests and saw both but it wasn't obvious when to choose one naming over the other. For this, I renamed to use `.verify.cpp` though, but I don't feel strongly and also don't know which is preferred. @cjdb @ldionne -- can one of you provide some insight on the naming?
> Deferring to @ldionne. +1 to `expected_error@*:*`, which is the libc++ way.
It should indeed be a `.verify.cpp` and `main` should be renamed to something else.

`utils/libcxx/test/format.py`

```
    FOO.verify.cpp          - Compiles with clang-verify. This type of test is
                              automatically marked as UNSUPPORTED if the compiler
                              does not support Clang-verify.
    
    FOO.fail.cpp            - Compiled with clang-verify if clang-verify is
                              supported, and equivalent to a .compile.fail.cpp
                              test otherwise. This is supported only for backwards
                              compatibility with the test suite.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108855