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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb 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:
> 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.


================
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>'}}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108855