[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 a comment.
This revision now requires changes to proceed.

> I haven't read all you comments @var-const. I updated the tests reflect the changes from D121435 <https://reviews.llvm.org/D121435>, that should address all your comments.

I think a few of those comments are still relevant (unless I'm missing something).



================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:26
+// no implicit conversion
+static_assert(!std::is_constructible_v<std::ranges::out_value_result<A, A>, std::ranges::out_value_result<int, int>>);
+
----------------
var-const wrote:
> Nit: a few of these lines are longer than 120 columns.
> 
> Optional: maybe use a type alias to cut down the boilerplate? I find it a little hard to separate the useful information in each check.
> ```
> template <class T, class U>
> using Res = std::ranges::out_value_result<T, U>; // Any short name would do
> ```
This is still relevant.


================
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:
> > > 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.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:64
+constexpr bool test() {
+  {
+    std::ranges::out_value_result<int, double> res{10L, 0.};
----------------
var-const wrote:
> Can you please add a comment to each test case that briefly explains what it does?
Still relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121436