[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 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>
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108855