[libcxx-commits] [PATCH] D113906: [libc++] [test] Add "robust_re_difference_type.compile.pass.cpp" for all the algorithms.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
Mordante wrote:
> My concern with this test is a bit, how can we keep it up-to-date when newer versions of the Standard add new algorithms.
> I don't see this as a blocker, but if you have any suggestions I'd like to hear them.
The ideal is, whenever someone implements a new algorithm (like `shift_left`), they do a quick `git grep shift_left` for all the places in the codebase that mention it, and fix up the places that need fixing up. Also, they do a quick `git grep inplace_merge` or whatever, on an algorithm that's //similar// to the one they're adding, and again fix up the places that need fixing up.
Obviously this ideal was not followed in practice even for `shift_left`, because there were still `TODO` comments left in this code. But I can't think of any protocol that would be //better// than the existing ideal. Just have to work on communicating the best practices and reminding people in code reviews.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:22
+template<class>
+struct Intable {
+    TEST_CONSTEXPR operator int() const { return 1; }
----------------
philnik wrote:
> I think a better name would be `ConvertibleToInt`.
Veto; sure it's only used once in the whole file, but still I see no reason to make the name longer and more technical.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:57
+    (void)std::for_each(first, last, [](void*){});
+#if TEST_STD_VER >= 17
+    (void)std::for_each_n(first, count, [](void*){});
----------------
Mordante wrote:
> Is there a reason to change from our usual pattern of `#if TEST_STD_VER > 14`?
> IIRC this question was raised in a review not that long ago, but I don't recall that we wanted to move to this style.
Well, this isn't a "change" for this particular piece of code; I did it with `>=` in D92776 for some reason.
I don't object to changing it from `>=` to `>` in this same PR, as long as we're confusing Phab anyway. :)


================
Comment at: libcxx/test/std/algorithms/robust_re_difference_type.compile.pass.cpp:28
+#define NOT_CONSTEVAL(...) __VA_ARGS__
+#endif
+
----------------
philnik wrote:
> In D110598 I define a `TEST_IS_CONSTANT_EVALUATED`. The `TEST_IS_CONSTANT_EVALUATED` is more verbose but I think it is the more general approach. And maybe feature test with `__cpp_lib_is_constant_evaluated` instead of assuming it in C++20 if you still want to go with `NOT_CONSTEVAL`.
I remembered something like that, but was unable to find it via `git grep`. Apparently because it wasn't checked in yet. :)

I'll merge that "test_macros.h" diff in here and start using it.

Btw, a problem I foresee, both here and (probably) there, is that very soon we'll need a macro that expresses "Test this only if `!is_constant_evaluated()`, or if the surrounding function isn't constexpr, //or// if `TEST_STD_VER > 20`," because some algorithms will be "non-constexpr-friendly in C++20 but constexpr-friendly in C++23".  We're going to need something like `if (TEST_IS_RUNTIME_OR_CXX20)`, `if (TEST_IS_RUNTIME_OR_CXX23)`, etc. We don't need to solve that //yet//, but it's worth seeing that the problem is coming.


================
Comment at: libcxx/test/std/algorithms/robust_re_difference_type.compile.pass.cpp:87
+    (void)std::copy(first, last, first2);
+    // TODO FIXME (void)std::copy_n(first, count, first2);
+    (void)std::copy_backward(first, last, last2);
----------------
Mordante wrote:
> Any idea why this one fails?
Yes: we fail to cast `count` to the difference_type. (The spec of the `_n` algorithms is quite weird; they don't accept something of `difference_type` right off the bat, they accept an arbitrary type and then convert it to "an" integral type.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113906