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

[Louis Dionne via Phabricator via libcxx-commits]

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127557