[libcxx-commits] [PATCH] D93562: [libc++] Add a missing `<_Compare>` template argument.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/algorithm:4500
     }
 }
 
----------------
@curdeius wrote:
> Hmm, just one thought, it could be tested using non-copyable comparators passed by (non-reduced) lvalue ref, nope? How did you find this "missed optimization"?

I believe comparators must be copyable by definition (or else it's library UB). I also believe it's unspecified how many times libc++ may copy them. //Traditionally// the STL copies comparators as liberally as it copies iterators or allocators; libc++'s reference dance is just for QoI. Arguably, any programmer who cares should be passing `std::ref(myComparator)` in the first place, instead of passing a heavyweight `myComparator` by value.

We could still add a QoI test for this in `libcxx/test/libcxx/`; leave the comparator copyable but have it count how many times it was copied, and then assert that the number of copies was 1. I'm not volunteering.

How did I find it? Just staring at the code. I've been staring at these helper-function calls a lot lately, because of constexpr algorithms and also because of ADL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93562