[libcxx-commits] [PATCH] D141216: [libc++][test] zip_view test cleanups

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter planned changes to this revision.
CaseyCarter added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/cpo.pass.cpp:66
 
+#ifdef _LIBCPP_VERSION // libc++ doesn't implement P2165R4 yet
     static_assert(std::is_same_v<std::ranges::range_reference_t<decltype(v2)>, std::tuple<std::pair<int&, int&>>>);
----------------
philnik wrote:
> Maybe guard this on `defined(__cpp_lib_tuple_like) && __cpp_lib_tuple_like >= 202207L`?
As Hui commented below, the intent isn't to keep both alternatives. My intent is that these tests break loudly upon implementing P2165R4 so someone will open them up and remove the "old" code instead of leaving it dead. We could equivalently have _only_ the P2165R4 behavior and XFAIL these tests for libc++ but I prefer to validate the "wrong" behavior for the time being to defend against regressions that would cause the test to fail but in a different way.

Full disclosure: MSVCSTL has been implementing P2165 piecemeal, so we don't define the feature-test macro yet. Guarding with the macro wouldn't accomplish my goal of getting all of these tests up on MSVC.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:53
     auto it = v.begin();
-    using Pair = std::pair<const int&, const int&>;
-    assert(*it++ == Pair(buff[0], buff[0]));
-    assert(*it++ == Pair(buff[1], buff[1]));
-    assert(*it == Pair(buff[2], buff[2]));
+#ifdef _LIBCPP_VERSION // libc++ doesn't implement P2165R4 yet
+    using Value = std::pair<const int&, const int&>;
----------------
huixie90 wrote:
> In theory I guess we should check the feature test macro `__cpp_lib_tuple_like`, but I think I prefer your solution. We are going to implement the paper without supporting the previous behaviour. so after implementing the paper, a test failure would be preferable over silently selecting another branch.
I argue for this approach in some detail in my response to Philnik's comment above.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/types.h:322
 
-  constexpr int& operator*() const { return *it_; }
+  constexpr auto& operator*() const { return *it_; }
 
----------------
huixie90 wrote:
> Thanks for the fix. I think this function is not used in the tests. The main usage of this class is to test the `end` function returns the correct type and compares with the iterator correctly (without calling operator*)
> btw, I prefer `decltype(auto)` here even though it does not really matter.
I used `auto&` to conservatively make this "just generic enough to cover existing uses" but I agree that full `decltype(auto)` is more completely future-proof.


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

https://reviews.llvm.org/D141216