[libcxx-commits] [PATCH] D121435: [libc++] Canonicalize the ranges results and their tests

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/in_fun_result.h:23
 
-#ifndef _LIBCPP_HAS_NO_CONCEPTS
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
----------------
I //think// this should be `#if _LIBCPP_STD_VER > 17` instead.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:10
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
----------------
I //think// `no-concepts` is no longer necessary.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:11
 // UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
 
----------------
Similar to the other patch, I'm concerned there appears to be a discrepancy between the guard used in the headers and the one used in the tests.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:60
+#ifdef TEST_COMPILER_CLANG
+#pragma clang diagnostic ignored "-Wunknown-attributes"
+#elif defined(TEST_COMPILER_GCC)
----------------
Question: why is it not possible to use `_LIBCPP_NO_UNIQUE_ADDRESS`?


================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:66
+  int a;
+  [[no_unique_address, msvc::no_unique_address]] std::ranges::min_max_result<Empty> b;
+  int c;
----------------
Can you add a comment to explain why `min_max_result` has a significantly different test (I presume because it's the only class where both member variables have the same type)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121435