[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 marked 17 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:22
+template<class>
+struct Intable {
+    TEST_CONSTEXPR operator int() const { return 1; }
----------------
ldionne wrote:
> Quuxplusone wrote:
> > 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.
> Honestly, `Intable` is a poor name IMO. I would actually do:
> 
> ```
> template <class T>
> struct ConvertibleTo { TEST_CONSTEXPR operator T() const { return T(); } };
> ```
Well, I definitely wouldn't make it a template. Tell you what, I'll change it to `ConvertibleToIntegral`. (That's the phrase in https://eel.is/c++draft/alg.copy#11 )


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:9
+
+// UNSUPPORTED: c++03
+
----------------
var-const wrote:
> Question: why doesn't this test apply to C++03?
Lambdas and braced-initializer-lists (for `std::min/max`), but really probably just I didn't originally feel like going through and ifdeffing all the algos that were "new in C++11." It was easier just to unsupport the test pre-C++11.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:45
+    (void)std::all_of(first, last, [](void*){ return true; });
+    (void)std::any_of(first, last, [](void*){ return true; });
+    (void)std::copy(first, last, first2);
----------------
var-const wrote:
> Do these tests not apply to any of the algorithms in `<numeric>`?
I suppose they could apply to all of the algorithms in `<numeric>`. I'll do that as a separate PR, since it opens a new can of worms (namely, whether it should be a new test or part of this test).


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:60
+#endif
+    (void)std::generate(first, last, [](){ return nullptr; });
+    (void)std::generate_n(first, count, [](){ return nullptr; });
----------------
var-const wrote:
> Ultranit: omit the empty parentheses?
Definitely not. :)


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:82
+    (void)std::rotate_copy(first, mid, last, first2);
+#if TEST_STD_VER > 17
+    (void)std::shift_left(first, last, count);
----------------
var-const wrote:
> Just checking: `random_shuffle` is omitted deliberately, right?
I don't remember my original logic, but yeah, since it's deprecated/removed, I think it's fine to omit.
`std::shuffle` is also omitted, and I think my logic in that case was just that it was too awkward to define a URBG just for that one call.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:146
+
+    // WITH COMPARATORS
+    (void)std::adjacent_find(first, last, std::equal_to<void*>());
----------------
var-const wrote:
> Optional: having "with comparators" and "without comparators" as two separate lists makes it harder to notice if the lists happen to go out of sync. I'd prefer having the both versions right next to each other, so that it's easier to spot if one version is accidentally omitted (because the overloads are always next to each other, rather than arbitrarily far away):
> ```
>     (void)std::adjacent_find(first, last);
>     (void)std::adjacent_find(first, last, std::equal_to<void*>());
>     (void)std::binary_search(first, last, value, std::less<void*>());
>     (void)std::equal(first, last, first2);
>     (void)std::equal(first, last, first2, std::equal_to<void*>());
> ```
> 
> This is purely a matter of taste, though -- feel free to ignore.
My original logic was that separate lists made it easier to detect when one of the calls was accidentally //omitting// (or accidentally including) the comparator.
However, when updating these lists in this PR, I did find it pretty awkward to figure out which algos belonged in which list(s); so yeah, I think I'll combine the lists.


================
Comment at: libcxx/test/std/algorithms/robust_re_difference_type.compile.pass.cpp:20
+#include <functional>
+#include <utility>
+
----------------
var-const wrote:
> Question: is `<utillity>` used?
I'd thought so, but I guess not. Added `<iterator>`, removed `<utility>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113906