This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jloser on Sep 12 2021, 7:13 PM.

Details

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.

Diff Detail

Event Timeline

jloser requested review of this revision.Sep 12 2021, 7:13 PM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2021, 7:13 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Sep 13 2021, 8:07 AM

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
37–38

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!

41

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

42–43

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

43–53

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

This revision now requires changes to proceed.Sep 13 2021, 8:07 AM
jloser added inline comments.Sep 13 2021, 8:31 AM
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 updated this revision to Diff 372299.Sep 13 2021, 11:22 AM
jloser edited the summary of this revision. (Show Details)

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 marked 4 inline comments as done.Sep 13 2021, 11:22 AM
jloser added inline comments.
libcxx/test/std/containers/views/span.cons/deduct.pass.cpp
43–53

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

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

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 updated this revision to Diff 372303.Sep 13 2021, 11:38 AM
jloser edited the summary of this revision. (Show Details)

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

jloser marked an inline comment as done.Sep 13 2021, 11:41 AM
jloser added inline comments.
libcxx/test/std/containers/views/span.cons/deduct.pass.cpp
43–53

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.

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

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 updated this revision to Diff 372305.Sep 13 2021, 11:46 AM

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

jloser marked 2 inline comments as done.Sep 13 2021, 11:46 AM
ldionne accepted this revision.Sep 20 2021, 1:49 PM

LGTM but we must fix the Debug mode CI!

This revision is now accepted and ready to land.Sep 20 2021, 1:49 PM
jloser updated this revision to Diff 373741.Sep 20 2021, 4:24 PM

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.

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.

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

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 requested changes to this revision.Sep 21 2021, 10:46 AM
ldionne added inline comments.
libcxx/test/std/containers/views/span.cons/deduct.pass.cpp
43–53

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.

This revision now requires changes to proceed.Sep 21 2021, 10:46 AM
jloser updated this revision to Diff 373987.Sep 21 2021, 10:55 AM

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

jloser updated this revision to Diff 374001.Sep 21 2021, 12:14 PM
jloser edited the summary of this revision. (Show Details)

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

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

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.

60–61

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

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

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

ldionne accepted this revision.Sep 21 2021, 3:32 PM

Thanks!

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

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.

This revision is now accepted and ready to land.Sep 21 2021, 3:32 PM
jloser updated this revision to Diff 374066.Sep 21 2021, 4:29 PM

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

jloser marked an inline comment as done.Sep 21 2021, 4:30 PM