[libcxx-commits] [PATCH] D120637: [libc++][ranges] Implement ranges::minmax and ranges::minmax_element

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:68
+      return {*__result.min, *__result.max};
+    }
+
----------------
philnik wrote:
> var-const wrote:
> > Should the rest of the function go into an `else` branch, to make sure it's part of `if constexpr`?
> I'm not a huge fan of that, but I agree that having faster compile times is more valuable than the bit of increased readability it gives to not have the else branch. Just for the record, I would definitely not want the else branch if it were a normal if.
I fully agree from the readability perspective. I would also prefer no `else` branch if it wasn't `if constexpr`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:156
+
+  { // check that a single element works
+    auto ret = std::ranges::minmax({ 1 });
----------------
Nit: some test cases place the comment on the same line as the opening brace, and some on the next line. I don't mind either, but it should be consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637