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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const 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 {
----------------
philnik wrote:
> 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.
I would agree to that if someone volunteers to do that follow-up in the near future -- in fact, it would be great. Would you or @avogelsgesang be willing to take this on?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395