[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:4487
+        _VSTD::__half_inplace_merge<_Compare>(__buff, __p, __middle, __last, __first, __comp);
     }
     else
----------------
@curdeius:
> Excuse my ignorance, but could you please explain why letting `_Compare` get deduced is wrong/suboptimal?
> IIUC the case you mention is when e.g. `__buffered_inplace_merge` has `_Compare` being some non-ref type `T`, so then calling `__half_inplace_merge` will deduce its `_Compare` to be an lvalue ref `T &`. So far so good. But why/when is it bad?

Other way around. (And I'm sure it's not your ignorance, just a brain fart. ;)) The trouble case is when `_Compare` is `T&`, and then passing `__comp` by value will make `__half_inplace_merge` deduce //its// `_Compare` as `T` and make a copy, when really we want `__half_inplace_merge`'s `_Compare` to also be `T&`.

Basically, libc++ algorithms have an invariant that "`_Compare` is //either// an lvalue reference type //or// `__debug_less<T&>` (which works like `reference_wrapper` and is cheap to copy)." It would actually be very nice if we could just make the invariant that "`_Compare` is a value type that is cheap to copy (e.g. `reference_wrapper<T>`)," and then we'd never need to specify `<_Compare>` on any helper ever again...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93562