[libcxx-commits] [PATCH] D131395: [libc++] Implement `lexicographical_compare_three_way`

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:100
+    return __lexicographical_compare_three_way_fast_path(
+        __first1, __last1, __first2, __last2, std::forward<_Cmp>(__comp));
+  } else {
----------------
var-const wrote:
> JohelEGP wrote:
> > avogelsgesang wrote:
> > > Mordante wrote:
> > > > Is `forward` intended? `__comp` is passed by value.
> > > good catch! Changed to `move`
> > I think `ref(__comp)` is the preferred way.
> @JohenEGP I presume you referring to the `__comp_ref_type`, right? It's a great suggestion.
> 
> @avogelsgesang For context, there is an existing pattern in algorithms where the internal implementation of the function wraps the comparator in a typedef that is defined differently in debug and non-debug modes (see `include/__algorithm/comp_ref_type.h`):
> ```
> #ifdef _LIBCPP_ENABLE_DEBUG_MODE
> template <class _Comp>
> using __comp_ref_type = __debug_less<_Comp>;
> #else
> template <class _Comp>
> using __comp_ref_type = _Comp&;
> #endif
> ```
> 
> So in non-debug modes, this resolves to simply a reference. In debug mode, however, it creates a temporary of a helper type `__debug_less` that additionally checks that the comparator in fact does induce a strict weak order.
> 
> I think we want to continue using this pattern going forward (e.g. `lexicographical_compare` uses it). The only issue is that the existing `__debug_less` only supports comparators returning a boolean. You would probably need to create a separate `__three_way_comp_ref_type` typedef and a separate `__debug_three_way_comp` helper struct (names are subject to change).
> 
> Please let me know if you'd like any additional context on this (this can be kinda confusing). Many existing algorithms can be used as examples of this pattern; a caveat is that more recent code uses a (very) slightly different approach:
> - older code tends to call have `_Compare` as a template parameter of the internal function, declare the parameter as `_Compare __comp`, and have the caller specify the template argument explicitly, like `std::__foo<_comp_ref_type<_Comp>>(__first, __last, __comp)`. See e.g. `lexicographical_compare`;
> - in newer code, the pattern was to declare the parameter as a forwarding reference `_Compare&& __comp` and have the caller do a `static_cast`, like `std::__foo(__first, __last, static_cast<__comp_ref_type(__comp)>` (see e.g. `inplace_merge`).
> 
> If you prefer to, I'm fine with doing this in a follow up since this patch has been open for a while now.
IMO we should make the debug stuff a separate effort. AFAIK we don't test it anywhere and because of that I'm pretty sure it regressed in some places. Instead of adding another untested branch, I'd suggest creating a patch that adds tests to all algorithms and, depending on scope, update the algorithms in follow-up patches or as part of the test-patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395