[libcxx-commits] [PATCH] D121436: [libc++][ranges] Add ranges::out_value_result

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/out_value_result.h:23
+
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
+
----------------
Please don't forget to remove this after rebasing (due to https://reviews.llvm.org/D132151).


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp:94
+
+  return 0;
 }
----------------
`return 0` should stay, unless there was a recent change regarding this that I'm not aware of.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:63
+
+constexpr bool test() {
+  {
----------------
var-const wrote:
> var-const wrote:
> > var-const wrote:
> > > var-const wrote:
> > > > A few test cases I'd like to add:
> > > > - check that `out_value_result` is trivial and standard layout when it is true of the arguments;
> > > > - check default construction;
> > > > - check that `out_value_result` is copyable and movable if the arguments are (I think a `static_assert` is enough);
> > > > - check that the rvalue overload of the conversion operator actually moves the arguments (by using a helper type that counts the number of copies and moves);
> > > > - check that the `[[no_unique_address]]` optimization is used;
> > > > - maybe check aggregate initialization (might be overkill).
> > > > check that the `[[no_unique_address]]` optimization is used; 
> > > Ah, I see that it's addressed in D121435.
> > I spoke too soon -- while D121435 adds a few such tests, it doesn't address `out_value_result`, so the original comment still stands.
> Still relevant.
> A few test cases I'd like to add:
> - check that `out_value_result` is trivial and standard layout when it is true of the arguments;
> - check default construction;
> - check that `out_value_result` is copyable and movable if the arguments are (I think a `static_assert` is enough);
> - check that the rvalue overload of the conversion operator actually moves the arguments (by using a helper type that counts the number of copies and moves);
> - check that the `[[no_unique_address]]` optimization is used;
> - maybe check aggregate initialization (might be overkill).

I think a few of these are still not addressed -- in particular, trivial check, default construction and that the arguments are actually moved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121436