[libcxx-commits] [PATCH] D127557: [libc++][ranges] Implement `ranges::sort`.

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Jun 16 11:00:14 PDT 2022

ldionne added inline comments.

Comment at: libcxx/include/__algorithm/ranges_common.h:6
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
`make_projected.h`? `ranges_common.h` means that this will likely become a dumping ground, which we're trying to avoid with the header "modularization".

Comment at: libcxx/include/__algorithm/ranges_common.h:30
+_LIBCPP_HIDE_FROM_ABI constexpr static
+auto __make_projected_pred(_Pred& __pred, _Proj& __proj) {
+  static_assert(!same_as<_Proj, identity>,
I would only add this when you actually need it, otherwise it's basically adding a never used function (without tests, because nothing's using it).

Comment at: libcxx/include/__algorithm/ranges_common.h:40-42
+auto __make_projected_comp(_Comp& __comp, _Proj& __proj) {
+  static_assert(!same_as<_Proj, identity>,
+      "If the projection is std::identity, just pass the comparator directly.");
How about this instead:

template <class _Comp, class _Proj>
_LIBCPP_HIDE_FROM_ABI constexpr static
decltype(auto) __make_projected_comp(_Comp& __comp, _Proj& __proj) {
  if constexpr (same_as<_Proj, identity>) {
    return __comp;
  } else {
    return <lambda>;

and then from the caller side, use:

auto&& __projected_comp = __make_projected_comp(__comp, __proj);

I'm not sure which one is better, but this is another option. The benefit with this version is that we can forego the `if constexpr` inside `__sort_fn_impl` (and possibly other algorithms in the future).

You can also centralize your comment about `// Avoid creating the lambda [...]` in `__sort_fn_impl` here.

Comment at: libcxx/include/__algorithm/sort.h:119
 template <class _Tp>
 struct __is_simple_comparator : false_type {};
While we're at it, can you please add a comment explaining what's the purpose of this trait?

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list