[libcxx-commits] [PATCH] D109668: [libc++][test] Fix iterator assertion in span.cons/deduct.pass.cpp

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/test/std/containers/views/span.cons/deduct.pass.cpp:26
 //   template<class Container>
 //     span(const Container&) -> span<const typename Container::value_type>;
 
----------------
cppreference says: https://en.cppreference.com/w/cpp/container/span/deduction_guides


================
Comment at: libcxx/test/std/containers/views/span.cons/deduct.pass.cpp:39-41
 // 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"
----------------
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!


================
Comment at: libcxx/test/std/containers/views/span.cons/deduct.pass.cpp:46
     {
     int arr[] = {1,2,3};
     std::span s{arr};
----------------
Notably missing: a test for `const int arr[]`.


================
Comment at: libcxx/test/std/containers/views/span.cons/deduct.pass.cpp:48-49
     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())));
----------------
Nit: `ASSERT_SAME_TYPE(decltype(s), std::span<int, 3>);` to avoid needlessly defining `S`.


================
Comment at: libcxx/test/std/containers/views/span.cons/deduct.pass.cpp:50
     ASSERT_SAME_TYPE(S, std::span<int, 3>);
     assert((std::equal(std::begin(arr), std::end(arr), s.begin(), s.end())));
     }
----------------
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>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109668