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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision now requires review to proceed.

Despite reading the discussion, I believe this test provides little benefit since we're testing something that's invalid. The correct way of testing that we use `std::to_address` would be to do something funky with `operator&` so that we'd see a failure if we were not using `std::addressof`. There's an infinite number of things that don't work that we could test, but we basically never do that unless it's really relevant.

If we keep the test, let's address a few comments. Otherwise, this LGTM and I don't need to see this again. Thanks!



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp:20
+
+// Expect failures with a type missing an `element_type` type alias
+
----------------
If we're going to keep this test, we should reformulate this comment. Also, move it above to replace the 

```
// template<class T>
// concept contiguous_iterator;
```

comment. I think this would be better:

```
// This test checks that std::contiguous_iterator uses std::to_address, which is not SFINAE-friendly
// when the type is missing the `T::element_type` typedef.
```

Also, we need to add `// REQUIRES: libc++` since this behavior is libc++ specific.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp:55
+    // In checking constraints in `std::contiguous_iterator`, the call to
+    // std::to_address` is not SFINAE-friendly, resulting in a hard error
+    // when used with a type missing an `element_type` type alias.
----------------
Missing back tick.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108855