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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM.



================
Comment at: libcxx/include/algorithm:4487
+        _VSTD::__half_inplace_merge<_Compare>(__buff, __p, __middle, __last, __first, __comp);
     }
     else
----------------
Quuxplusone wrote:
> @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...
> Other way around. (And I'm sure it's not your ignorance, just a brain fart. ;))
Indeed, I somehow forcingly made myself think that the rule "3) If P is a reference type, the type referred to by P is used for deduction." for argument deduction is non existent.
Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93562