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

[jloser]

Summary

contiguous_iterator requires the iterator type passed is either a
pointer type or that the element type of the iterator is a complete
object type. These constraints are not part of the current wording in
defining the contiguous_iterator concept - adjust the concept to
reflect this.

Inspired from discussion at https://reviews.llvm.org/D108645.

[cjdb]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

133

This test needs to remain.

167

As does this one.

[jloser]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

167

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.

[jloser]

Add element_type type alias to wrong_iter_reference_t to avoid hard error in std::to_address constraint evaluation from std::contiguous_iterator concept.

[jloser]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

133

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?

[cjdb]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

133

Thank you!

167

Ack, but the test is still useful. Please move this to a verification test so we can keep it.

[jloser]

[NFC] Fix formatting of element_type type alias.

[• Quuxplusone]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

105–107

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.

133

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.

167

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

[cjdb]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

133

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.

This comment seems to be misplaced, but I will note that just because Microsoft/STL and libstdc++ chose not to be SFINAE-friendly has no bearing on whether or not we choose to be SFINAE-friendly, if we're allowed to be.

167

The point in checking this is to improve confidence that std::to_address is actually used. Please do not remove this test.

[jloser]

Swap pointer and reference types for wrong_iter_reference_t so its semantics match its name

[jloser]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

105–107

Valid point; just changed it so the type's reference type is the "wrong" one so its name matches reality.

[jloser]

Add test showing non-SFINAE-friendly std::to_address call is used in contiguous_iterator concept

[jloser]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

167

I added a new test file expecting the appropriate note/errors around use of std::to_address when evaluating std::contiguous_iterator concept for a type that is missing an element_type type alias.

[jloser]

[NFC] Update commit message

[Mordante]

In general LGTM, but I like to see the build pass before approving.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp

15 ↗

(On Diff #369303)

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

[• Quuxplusone]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp

15 ↗

(On Diff #369303)

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.

57–58 ↗

(On Diff #369303)

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@__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.

[jloser]

Rename test to use .verify in filename rather than .fail
Simplify regexes for expected-note and expected-error

[jloser]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp

57–58 ↗

(On Diff #369303)

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?

[cjdb]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp

15 ↗

(On Diff #369303)

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.

57–58 ↗

(On Diff #369303)

Deferring to @ldionne. +1 to expected_error@*:*, which is the libc++ way.

[• Quuxplusone]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp

15 ↗

(On Diff #369303)

Ah, right, std::contiguous_iterator is defined in <iterator>, not <concepts>. OK.

[• Quuxplusone]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp

15–18

#include <iterator>
#include <compare>
#include <cstddef>

@cjdb points out that contiguous_iterator comes from <iterator>, not <concepts>; thus <iterator> is the header-under-test here. Oops.

57–58

My earlier comment was meant to implicitly apply to both expected- thingies. And let's reorganize the lines while we're at it:

(void) std::contiguous_iterator<no_element_type>;
// expected-error@*:* {{implicit instantiation of undefined template}}
// expected-note@*:* {{to_address}}

[jloser]

Fix includes to show <iterator> is the header under test.
Move expected-error and expected-note to be on separate lines.

[ldionne]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp

12

I was going to make some comments about things that need to change in this test, however now I'm not sure we need it at all. Do we really need to test that something that doesn't work doesn't work?

Things might be different if we were testing a specific failure diagnostic, but as it stands, I don't really understand what test coverage this is providing.

[jloser]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp

12

Right, I originally removed the test as I mentioned it was ill-formed due to the type missing an element_type type alias, but @cjdb wanted to keep the test.

@cjdb can you share some insight into the value you see in keeping this as a verification test?

[jloser]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp

12

@ldionne see https://reviews.llvm.org/D108855?id=369228#inline-1037144 for some context on the previous discussion regarding this test.

[Mordante]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.fail.cpp

15 ↗

(On Diff #369303)

You're right.

57–58 ↗

(On Diff #369303)

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.

[cjdb]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp

12

That pretty much sums it up. I don't think there's another test that checks std::to_address is a hard requirement.

[ldionne]

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!

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp

21

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.

56

Missing back tick.

[jloser]

[NFC] Clean up test comments
Mark test as requiring libc++

libcxx/include/__iterator/concepts.h

concept random_access_iterator =
   };
 
 template<class _Ip>
 concept contiguous_iterator =
   random_access_iterator<_Ip> &&
   derived_from<_ITER_CONCEPT<_Ip>, contiguous_iterator_tag> &&
   is_lvalue_reference_v<iter_reference_t<_Ip>> &&
   same_as<iter_value_t<_Ip>, remove_cvref_t<iter_reference_t<_Ip>>> &&
-  (is_pointer_v<_Ip> || requires { sizeof(__pointer_traits_element_type<_Ip>); }) &&
   requires(const _Ip& __i) {
     { _VSTD::to_address(__i) } -> same_as<add_pointer_t<iter_reference_t<_Ip>>>;
   };
 
 template<class _Ip>
 concept __has_arrow = input_iterator<_Ip> && (is_pointer_v<_Ip> || requires(_Ip __i) { __i.operator->(); });
 
 // [indirectcallable.indirectinvocable]

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

 };
 
 static_assert(std::random_access_iterator<mismatch_value_iter_ref_t>);
 static_assert(!std::contiguous_iterator<mismatch_value_iter_ref_t>);
 
 struct wrong_iter_reference_t {
     typedef std::contiguous_iterator_tag    iterator_category;
     typedef short                           value_type;
+    typedef short                           element_type;
     typedef std::ptrdiff_t                  difference_type;
-    typedef int*                            pointer;
-    typedef short&                          reference;
+    typedef short*                          pointer;
+    typedef int&                            reference;
     typedef wrong_iter_reference_t          self;
 
     wrong_iter_reference_t();
 
     reference operator*() const;
     pointer operator->() const;
     auto operator<=>(const self&) const = default;
 
struct wrong_iter_reference_t {
     self& operator-=(difference_type n);
     self operator-(difference_type n) const;
     difference_type operator-(const self& n) const;
 
     reference operator[](difference_type n) const;
 };
 
 static_assert(std::random_access_iterator<wrong_iter_reference_t>);
 static_assert(!std::contiguous_iterator<wrong_iter_reference_t>);
 
-struct no_element_type {
-    typedef std::contiguous_iterator_tag    iterator_category;
-    typedef int                             value_type;
-    typedef std::ptrdiff_t                  difference_type;
-    typedef int*                            pointer;
-    typedef int&                            reference;
-    typedef no_element_type                 self;
-
-    no_element_type();
-
-    reference operator*() const;
-    pointer operator->() const;
-    auto operator<=>(const self&) const = default;
-
-    self& operator++();
-    self operator++(int);
-
-    self& operator--();
-    self operator--(int);
-
-    self& operator+=(difference_type n);
-    self operator+(difference_type n) const;
-    friend self operator+(difference_type n, self x);
-
-    self& operator-=(difference_type n);
-    self operator-(difference_type n) const;
-    difference_type operator-(const self& n) const;
-
-    reference operator[](difference_type n) const;
-};
-
-static_assert(std::random_access_iterator<no_element_type>);
-static_assert(!std::contiguous_iterator<no_element_type>);
-
 struct to_address_wrong_return_type {
     typedef std::contiguous_iterator_tag    iterator_category;
     typedef int                             value_type;
     typedef int                             element_type;
     typedef std::ptrdiff_t                  difference_type;
     typedef int*                            pointer;
     typedef int&                            reference;
     typedef to_address_wrong_return_type    self;

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// template<class T>
+// concept contiguous_iterator;
+
+#include <concepts>
+#include <compare>
+#include <cstddef>
+#include <iterator>
+
+// Expect failures with a type missing an `element_type` type alias
+
+struct no_element_type {
+    typedef std::contiguous_iterator_tag    iterator_category;
+    typedef int                             value_type;
+    typedef std::ptrdiff_t                  difference_type;
+    typedef int*                            pointer;
+    typedef int&                            reference;
+    typedef no_element_type                 self;
+
+    no_element_type();
+
+    reference operator*() const;
+    pointer operator->() const;
+    auto operator<=>(const self&) const = default;
+
+    self& operator++();
+    self operator++(int);
+
+    self& operator--();
+    self operator--(int);
+
+    self& operator+=(difference_type n);
+    self operator+(difference_type n) const;
+    friend self operator+(difference_type n, self x);
+
+    self& operator-=(difference_type n);
+    self operator-(difference_type n) const;
+    difference_type operator-(const self& n) const;
+
+    reference operator[](difference_type n) const;
+};
+
+void test() {
+    // 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.
+    (void) std::contiguous_iterator<no_element_type>; // expected-note@__iterator/concepts.h:*  {{to_address}}
+    // expected-error@*:* {{implicit instantiation of undefined template}}
+}