[libcxx-commits] [PATCH] D128744: [libc++][ranges] Implement `ranges::partial_sort`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:105-135
+  auto check = []<class Iter, class Sent> {
+    // Empty sequence.
+    test_one<Iter, Sent, 0>({}, 0, {});
+
+    // 1-element sequence.
+    test_all_subsequences<Iter, Sent>(std::array{1});
+
----------------
var-const wrote:
> philnik wrote:
> > var-const wrote:
> > > philnik wrote:
> > > > Could you put this into a function? The calls below look really weird.
> > > This was suggested by @ldionne. The advantage is to have all the related code in the same scope, i.e. to avoid the details spilling out of the function.
> > By that logic, shouldn't everything be in nested lambdas in tests? You aren't capturing anything, so I really don't see how that's better than having it as an extra function. What exactly do you think is "spilling out of the function"?
> Let's leave this debate for another day, after the release. Changed.
> By that logic, shouldn't everything be in nested lambdas in tests? You aren't capturing anything, so I really don't see how that's better than having it as an extra function. What exactly do you think is "spilling out of the function"?

FWIW the idea is that a top-level function exposes a name, whereas a lambda doesn't because it's a local variable in another scope. It makes it clearer that it's only an implementation detail of e.g. `test_iterators()` used only for avoiding code duplication. I don't mind which approach we take given that we have more important stuff to focus on right now, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744