[libc++][test] Fix iterator assertion in span.cons/deduct.pass.cpp

[jloser]

Summary

Two tests in span.cons/deduct.pass.cpp accidentally check whether the
iterator range from member begin and member end are equivalent to the
ones from free begin and free end. This is obviously true and not
intended. Correct the intent by comparing the size/data from the span
with the source input.

While in the neighborhood, add test for const int arr[N], remove extraneous
type aliases, unused <type_traits> header, and the
disable_missing_braces_warning.h include.

[• Quuxplusone]

The initial idea of fixing this test is great. The current PR doesn't go nearly far enough.

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

26

cppreference says: https://en.cppreference.com/w/cpp/container/span/deduction_guides

38–39

This test has several (pre-existing) issues.
For one, obviously this test has nothing to do with std::array, and so if the compiler is complaining about a lack of double-braces, the right fix is just to add those double-braces — not to suppress the warning!

42

Notably missing: a test for const int arr[].

43–44

Nit: ASSERT_SAME_TYPE(decltype(s), std::span<int, 3>); to avoid needlessly defining S.

44–54

For another, the right assertion here and throughout is actually

assert(s.begin() == arr.begin());
assert(s.end() == arr.end());

That is, we should verify that the span gets the right settings for its members, in terms of the original array, not just that it's viewing onto some array with the same contents as the original.
As a bonus, this eliminates the test's dependency on <algorithm>.

[jloser]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

26

I agree that's where we'll get to. This was noticed as I was working on P1394 (https://wg21.link/p1394) which removes these container/const container deduction guides (and constructors) in favor of just a range constructor. So, the comment in the deduction test is fine/correct given the current implementation of span in libc++.

I'll clean up the other parts of the rest per your comments in this patch.

[jloser]

Add test for const int[N].
Remove extra type aliases in favor of inlining decltype call in ASSERT_SAME_TYPE
Remove unused <type_traits> include
Remove disable_missing_braces_warning.h include

[jloser]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

I like that idea, but I don't think that "just works" because of how span's iterators are __wrap_iter depending on _LIBCPP_DEBUG_LEVEL, so there won't be a operator== to make that valid. Example error:

error: invalid operands to binary expression ('int *' and 'std::span<int, 3>::iterator' (aka '__wrap_iter<int *>'))
    assert(std::begin(arr) == s.begin());

Is there something I'm missing to make this work regardless of whether the span's iterator type is pointer or __wrap_iter<pointer>?

[• Quuxplusone]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

Ah, gross. Well, since this is C++20, we can certainly do

assert(std::to_address(s.begin()) == arr.begin());
assert(std::to_address(s.begin()) == std::to_address(str.begin()));

etc. I guess this does explain why it wasn't written that way to begin with; we implemented span probably long before we had a working std::to_address. But these days we could use std::to_address, unless @ldionne objects for any reason.

[jloser]

Use std::to_address instead of std::equal everywhere in the test.

[jloser]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

Seems reasonable. I just switched to using std::to_address. I like it slightly better than the std::equal status quo -- only "slightly" because of the noise of not being able to just write assert(s.begin() == arr.begin()); for example.

[• Quuxplusone]

LGTM once buildkite is happy.

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

31

#include <memory> — the Modules build won't like that you're including a private header.

85–86

The cast to (size_t) shouldn't be needed anymore. I believe it's a relic of the great size_t/ssize_t wars. :)

[jloser]

Include <memory> instead of <__memory/pointer_traits.h> to make modules build happy
Remove cast to size_t

[ldionne]

LGTM but we must fix the Debug mode CI!

[jloser]

Workaround std::to_address(str.end()) hitting assert in test. There is an issue with the debug wrapped iterator for std::string::end(), but it's not clear why.

[jloser]

In D109668#3010463, @ldionne wrote:

LGTM but we must fix the Debug mode CI!

I dug into it for a bit, but I'm not sure how to fix it. The problem is from the statement std::to_address(str.end()). What we want to happen is str.end() to return the wrapped iterator and then pick up the std::to_address defined for wrapped iterators (so it calls its base() function). I don't know why this fires the asserts in __iterator/wrap_iter:95. Since the problem seems mostly unrelated to what the intent of the fix was in this patch, I recommend we fix it in a follow-up if @ldionne or @Quuxplusone know what's wrong. So, I brought back the std::equal call for the std::string and const std::string tests.

Let me know what you think.

[• Quuxplusone]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

I dug into it for a bit, but I'm not sure how to fix it. The problem is from the statement std::to_address(str.end()). What we want to happen is str.end() to return the wrapped iterator and then pick up the std::to_address defined for wrapped iterators (so it calls its base() function). I don't know why this fires the asserts in __iterator/wrap_iter:95

std::to_address(it) ends up calling it.operator->(), which contains that assertion, right?
I see an overload of std::__to_address in __iterator/wrap_iter.h, but I bet it's being #included after __memory/pointer_traits.h so that that overload isn't in scope at the right time. Maybe it would work to replace __iterator/wrap_iter.h's "overload of __to_address" with "partial specialization of pointer_traits." I could test that tomorrow, if nobody beats me to it.

[ldionne]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

std::to_address(it) ends up calling it.operator->(), which contains that assertion, right?

I haven't dug into it, but that was my hunch when I saw the CI failure, too.

Actually, thinking about it some more, it's not valid to call std::to_address on a std::string::iterator, because those are not required to be raw pointers (or even fancy pointers). I believe we should instead be checking that span.data() == source.data() and span.size() == source.size(). That provides us with the same level of coverage for checking that std::span's constructor does its job, but avoids relying on implementation details of libc++'s iterators.

[jloser]

Use std::data() for the two string tests to avoid std::to_address issues with string iterators in debug mode.

[jloser]

Compare size/data for all tests, i.e. avoid std::to_address with span iterators.

[• Quuxplusone]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

thinking about it some more, it's not valid to call std::to_address on a std::string::iterator, because those are not required to be raw pointers (or even fancy pointers).

They are required (in C++20) to be contiguous iterators, which means calling std::to_address on them must be valid. In C++20 there is essentially no difference between a "contiguous iterator" and a "fancy pointer."
https://en.cppreference.com/w/cpp/iterator/contiguous_iterator — particularly the semantic requirement involving std::to_address(c)

I believe we should instead be checking that span.data() == source.data() and span.size() == source.size().

I agree that that would serve the same purpose, for this test's purposes, so sure, let's do that and ship this. But we should still fix our std::to_address, orthogonally.

61–62

FWIW, you could use arr.size() and arr.data() here as well; std::array has those.

[• Quuxplusone]

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

I've taken a stab at this in D110198. I'm kinda relying on buildkite to tell me if it works, though. 😛

[ldionne]

Thanks!

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

44–54

They are required (in C++20) to be contiguous iterators, which means calling std::to_address on them must be valid.

Ugh, I missed that. Still, I think the consistency we have with the patch in its current state is best.

[jloser]

Use member size() and data() functions for std::array test cases

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp

 //
 //   template<class T, size_t N>
 //     span(const array<T, N>&) -> span<const T, N>;
 //
 //   template<class Container>
 //     span(Container&) -> span<typename Container::value_type>;
 //
 //   template<class Container>
 //     span(const Container&) -> span<const typename Container::value_type>;
 
 
 
 #include <span>
 #include <algorithm>
 #include <array>
 #include <cassert>
+#include <memory>
 #include <string>
-#include <type_traits>
 
 #include "test_macros.h"
 
-// std::array is explicitly allowed to be initialized with A a = { init-list };.
-// Disable the missing braces warning for this reason.
-#include "disable_missing_braces_warning.h"
-
 int main(int, char**)
 {
     {
     int arr[] = {1,2,3};
     std::span s{arr};
-    using S = decltype(s);
-    ASSERT_SAME_TYPE(S, std::span<int, 3>);
-    assert((std::equal(std::begin(arr), std::end(arr), s.begin(), s.end())));
+    ASSERT_SAME_TYPE(decltype(s), std::span<int, 3>);
+    assert(std::to_address(s.begin()) == std::begin(arr));
+    assert(std::to_address(s.end()) == std::end(arr));
+    }
+
+    {
+    const int arr[] = {1,2,3};
+    std::span s{arr};
+    ASSERT_SAME_TYPE(decltype(s), std::span<const int, 3>);
+    assert(std::to_address(s.begin()) == std::begin(arr));
+    assert(std::to_address(s.end()) == std::end(arr));
     }
 
     {
     std::array<double, 4> arr = {1.0, 2.0, 3.0, 4.0};
     std::span s{arr};
-    using S = decltype(s);
-    ASSERT_SAME_TYPE(S, std::span<double, 4>);
-    assert((std::equal(std::begin(arr), std::end(arr), s.begin(), s.end())));
+    ASSERT_SAME_TYPE(decltype(s), std::span<double, 4>);
+    assert(std::to_address(s.begin()) == arr.begin());
+    assert(std::to_address(s.end()) == arr.end());
     }
 
     {
     const std::array<long, 5> arr = {4, 5, 6, 7, 8};
     std::span s{arr};
-    using S = decltype(s);
-    ASSERT_SAME_TYPE(S, std::span<const long, 5>);
-    assert((std::equal(std::begin(arr), std::end(arr), s.begin(), s.end())));
+    ASSERT_SAME_TYPE(decltype(s), std::span<const long, 5>);
+    assert(std::to_address(s.begin()) == arr.begin());
+    assert(std::to_address(s.end()) == arr.end());
     }
 
     {
     std::string str{"ABCDE"};
     std::span s{str};
-    using S = decltype(s);
-    ASSERT_SAME_TYPE(S, std::span<char>);
-    assert((size_t)s.size() == str.size());
-    assert((std::equal(s.begin(), s.end(), std::begin(s), std::end(s))));
+    ASSERT_SAME_TYPE(decltype(s), std::span<char>);
+    assert(s.size() == str.size());
+    assert(std::to_address(s.begin()) == std::to_address(str.begin()));
+    //assert(std::to_address(s.end()) == std::to_address(str.end())); // FIXME: hits assert with debug iterators for str.end() in __iterator/wrap_iter.h
+    assert((std::equal(s.begin(), s.end(), str.begin(), str.end())));
     }
 
     {
     const std::string str{"QWERTYUIOP"};
     std::span s{str};
-    using S = decltype(s);
-    ASSERT_SAME_TYPE(S, std::span<const char>);
-    assert((size_t)s.size() == str.size());
-    assert((std::equal(s.begin(), s.end(), std::begin(s), std::end(s))));
+    ASSERT_SAME_TYPE(decltype(s), std::span<const char>);
+    assert(s.size() == str.size());
+    assert(std::to_address(s.begin()) == std::to_address(str.begin()));
+    //assert(std::to_address(s.end()) == std::to_address(str.end())); // FIXME: hits assert with debug iterators for str.end() in __iterator/wrap_iter.h
+    assert((std::equal(s.begin(), s.end(), str.begin(), str.end())));
     }
 
   return 0;
 }