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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:9
+
+// UNSUPPORTED: c++03
+
----------------
Question: why doesn't this test apply to C++03?


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:33
+{
+    Tester t {};
+    Tester u {};
----------------
Optional nit: maybe initialize `data` inside the struct instead?


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:44
+
+    (void)std::all_of(first, last, [](void*){ return true; });
+    (void)std::any_of(first, last, [](void*){ return true; });
----------------
Optional nit: functors are repeated a lot across the test, maybe create variables for them as well? It would also be consistent with how the test uses named variables like `first` and `last` instead of `t.data`, `t.data + 10`, etc.:
```
(void)std::all_of(first, last, pred);
(void)std::for_each(first, last, apply);
(void)std::generate(first, last, generator);
(void)std::is_sorted(first, last, less);
// ...
```


================
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);
----------------
Do these tests not apply to any of the algorithms in `<numeric>`?


================
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; });
----------------
Ultranit: omit the empty parentheses?


================
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);
----------------
Just checking: `random_shuffle` is omitted deliberately, right?


================
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*>());
----------------
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.


================
Comment at: libcxx/test/std/algorithms/robust_re_difference_type.compile.pass.cpp:20
+#include <functional>
+#include <utility>
+
----------------
Question: is `<utillity>` used?


================
Comment at: libcxx/test/std/algorithms/robust_re_difference_type.compile.pass.cpp:25
+template<class It, class DifferenceType>
+class PickyIterator {
+    It it_;
----------------
Can you please add a comment with a brief explanation of what is special about this iterator class?


================
Comment at: libcxx/test/std/algorithms/robust_re_difference_type.compile.pass.cpp:28
+public:
+    using iterator_category = std::random_access_iterator_tag;
+    using value_type = typename std::iterator_traits<It>::value_type;
----------------
Nit: include `<iterator>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113906