[libcxx-commits] [PATCH] D114136: [libc++] Test that our algorithms never copy a user-provided comparator.

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

Quuxplusone marked 5 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp:53
+    TEST_CONSTEXPR NullaryValue& operator=(const NullaryValue&) = default;
+    TEST_CONSTEXPR decltype(nullptr) operator()() const { return nullptr; }
+};
----------------
jloser wrote:
> Are you using `decltype(nullptr)` instead of `nullptr_t` because of C++03 support or something else?
Out of a completely mistaken set of beliefs that (1) Clang's C++03 mode supported `decltype` as an extension, (2) Clang's C++03 mode supported `nullptr` as an extension, (3) libc++'s C++03 mode did //not// define `std::nullptr_t`.
It turns out that Clang does //not// support `decltype` and `nullptr` in C++03; those are //macros// provided by libc++! And libc++ //does// provide `std::nullptr_t`. So I'll just switch to that. Thanks.

(In my own code I actually do use `decltype(nullptr)` out of laziness, because it's core-language and I can't be arsed to remember which header defines the library alias `std::nullptr_t`. But in this patch-series Louis has already warned me against laziness. ;))


================
Comment at: libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp:9
+
+// <algorithm>
+
----------------
var-const wrote:
> Same comment re. `<numeric>` as the other patch.
Ack. Same answer: that it's a good idea and I should do it for //all// these (now three) tests but in a different PR.


================
Comment at: libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp:16
+
+struct Less {
+    int *copies_;
----------------
var-const wrote:
> Optional: consider using a mixin to reduce boilerplate:
> ```
> struct CopyTracker {
>   int *copies_;
>   TEST_CONSTEXPR explicit CopyTracker(int *copies) : copies_(copies) {}
>   TEST_CONSTEXPR CopyTracker(const CopyTracker& rhs) : copies_(rhs.copies_) { *copies_ += 1; }
> };
> 
> struct Less : CopyTracker {
>   TEST_CONSTEXPR explicit Less(int *copies) : CopyTracker(copies) {}
>   TEST_CONSTEXPR bool operator()(void*, void*) const { return false; }
> };
> 
> // ...
> ```
> 
I find "minimize layers of abstraction" to be the best rule for test code. We want to make sure that when it goes wrong, it goes wrong for the right reasons, and not [slight caricature here] because we messed up some template metaprogramming somewhere in the middle of the test. :)  It's kinda the same reason I'm not a big fan of reusable helper headers.


================
Comment at: libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp:170
+    (void)std::sort_heap(first, last, Less(&copies)); assert(copies == 0);
+    if (!TEST_IS_CONSTANT_EVALUATED) (void)std::stable_partition(first, last, UnaryTrue(&copies)); assert(copies == 0);
+    if (!TEST_IS_CONSTANT_EVALUATED) (void)std::stable_sort(first, last, Less(&copies)); assert(copies == 0);
----------------
jloser wrote:
> var-const wrote:
> > Nit: I think LLVM style guide is 80 columns. Also, this would make the `assert` unconditional (applies below as well).
> `libc++`  has its own `.clang-format` with a `ColumnLimit` of `120` FWIW.
Unconditional `assert` detected by GCC buildkite, and fixed; thanks.
Line-length ugly, but doubling the number of lines strikes me as strictly uglier, so I'm calling this fine.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114136