[libcxx-commits] [PATCH] D116278: [libc++][ranges] Add ranges::in_in_result

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Mostly looks good, but there are some remaining issues.



================
Comment at: libcxx/include/__algorithm/in_in_result.h:36
+  constexpr operator in_in_result<_II1, _II2>() && {
+    return {std::move(in1), std::move(in2)};
+  }
----------------



================
Comment at: libcxx/include/module.modulemap:250
       module includes                 { private header "__algorithm/includes.h" }
+      module in_in_result             { private header "__algorithm/in_in_result.h" }
       module inplace_merge            { private header "__algorithm/inplace_merge.h" }
----------------
This should be one line below, like in the header.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:60
+int main(int, char**) {
+  {
+    std::ranges::in_in_result<int, double> res{10L, 0.};
----------------
Can you also test the contents of `main` as `constexpr` ?


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:74
+  }
+}
----------------



================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:17
+// [[no_unique_address]] for [algorithms.results]
+
+#include <algorithm>
----------------
Since this test is in `std` instead of `libcxx` I think it would be good to mention the class may only have the two members specified in the standard.
http://eel.is/c++draft/algorithms.results#1


================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:24
+static_assert(sizeof(std::ranges::in_in_result<int, Empty>) == sizeof(int));
+static_assert(sizeof(std::ranges::in_in_result<Empty, Empty>) == 2 * sizeof(Empty));
----------------
Why `2 * sizeof(Empty)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116278