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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

In D129741#3651996 <https://reviews.llvm.org/D129741#3651996>, @philnik wrote:

> I think we should run the `dangling` tests and check their outputs. In theory we could optimize on an algorithms returning `ranges::dangling`. For some algorithms this would work perfectly, but for others there are side-effects where the current tests would catch the problem while this test doesn't. The `nonbool` and `invoke` changes LGTM.

Made this a runtime test and added a TODO to check the non-dangling iterators. I'm not removing any existing tests yet, so this patch is strictly improving coverage even without checking the results (and I won't remove existing checks without adding an equivalent check here). Quite a few existing tests either don't check for dangling at all or just check the return type.

I'm not convinced this check is worthwhile, by the way -- I can't think of a case where an algorithm would produce incorrect output in presence of `dangling` but work correctly otherwise. But if these checks could be made very brief, I can add them (or won't object if someone else does).



================
Comment at: libcxx/include/__algorithm/ranges_remove_if.h:23
 #include <__ranges/concepts.h>
+#include <__ranges/dangling.h>
 #include <__ranges/subrange.h>
----------------
huixie90 wrote:
> 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`?
I simply forgot that `borrowed_subrange_t` isn't defined in `dangling.h`. Thanks for catching this!


================
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:
> 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)
Re. `dangling_both` -- I thought about it and it seemed a little overkill, but since you mentioned it, I decided to add it anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129741