[libcxx-commits] [PATCH] D129741: [libc++][ranges][NFC] Consolidate range algorithm checks for returning `dangling`.

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_remove_if.h:23
 #include <__ranges/concepts.h>
+#include <__ranges/dangling.h>
 #include <__ranges/subrange.h>
----------------
question. why do we need to include this header.  `borrowed_subrange_t` is defined in `subrange.h`. can we consider `dangling` is a detail in `borrowed_subrange_t`?


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_dangling.compile.pass.cpp:34
+  template <size_t N>
+  constexpr explicit NonBorrowedRange(std::array<int, N> arr) : NonBorrowedRange{arr.data(), arr.size()} {}
+  constexpr NonBorrowedRange(int* d, size_t s) : data_{d}, size_{s} {}
----------------
this is not an issue for the test because the test doesn't actually run the code. but this is undefined behaviour.
the `data_` pointer points to the temporary  copy `std::array arr` and will be dangling immediately after the constructor returns

This makes me think that we should probably actually run the code?


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_dangling.compile.pass.cpp:35
+  constexpr explicit NonBorrowedRange(std::array<int, N> arr) : NonBorrowedRange{arr.data(), arr.size()} {}
+  constexpr NonBorrowedRange(int* d, size_t s) : data_{d}, size_{s} {}
+
----------------
is this one used apart from called by the other constructor?


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_dangling.compile.pass.cpp:53
+void dangling_2nd(Func&& func, Input& in1, Input& in2, Args&& ...args) {
+  using ResultT = decltype(func(in1, R(in2), std::forward<Args>(args)...));
+  static_assert(std::same_as<ResultT, ExpectedT>);
----------------
a possible additional to the test is `dangling_both`


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_dangling.compile.pass.cpp:53
+void dangling_2nd(Func&& func, Input& in1, Input& in2, Args&& ...args) {
+  using ResultT = decltype(func(in1, R(in2), std::forward<Args>(args)...));
+  static_assert(std::same_as<ResultT, ExpectedT>);
----------------
huixie90 wrote:
> a possible additional to the test is `dangling_both`
perhaps it is outside the scope of this test, but for the existing tests in set operations, for those tests that are testing that the second argument is dangling, it also verifies the first argument is a normal iterator which points to the correct place. (but it might be over-testing)


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_dangling.compile.pass.cpp:155
+  dangling_2nd<set_intersection_result<int*, dangling, int*>>(std::ranges::set_intersection, in, in2, out);
+  //dangling_1st<set_symmetric_difference_result<dangling, int*>>(std::ranges::set_symmetric_difference, in, in2, out);
+  //dangling_1st<set_union_result<dangling, int*, int*>>(std::ranges::set_union, in, in2, out);
----------------
you can enable this test if you rebase the main. also I think this one also needs to test the 2nd case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129741