[libcxx-commits] [PATCH] D130330: [libc++][NFC] Add checks for lifetime issues in classic algorithms.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this!

In general I'm quite happy, but I would like to add test to catch "read after move" too.

I agree the current approach technically is UB, but I don't dislike it. I've seen this approach used before for life-time tracking.



================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:54
+    int i_;
+    bool moved_from_ = false; // Check for double moves.
+
----------------



================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:65
+
+    Value(Value&& rhs) TEST_NOEXCEPT : i_(rhs.i_) {
+      assert(lifetime_cache.contains(&rhs));
----------------
This can be `noexcept` since we only support C++11 and later.


================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:526
+
+TEST_CONSTEXPR int to_int(int x) { return x; }
+int to_int(LifetimeIterator::Value x) { return x.i_; }
----------------
This can be just `constexpr`.


================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:648
+#if TEST_STD_VER > 17 // Most algorithms are only `constexpr` starting from C++20.
+  static_assert(test<ConstexprIterator>(), "");
+#endif
----------------



================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:653
+int main(int, char**) {
+  test_all();
+
----------------
Do you expect `test_all` to grow? Otherwise it could be folded in main.


================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:9
+
+// UNSUPPORTED: c++03
+
----------------
var-const wrote:
> Making this support C++03 would be pretty painful because it doesn't support initializer lists.
Can you add that as a comment in the test itself?


================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:551
+
+  test(simple_in, [&](I b, I e) { std::any_of(b, e, is_neg); });
+  test(simple_in, [&](I b, I e) { std::all_of(b, e, is_neg); });
----------------
var-const wrote:
> I don't know of a good way to pass a pointer to a template function, so unlike similar tests for range algorithms I'm using lambdas here.
You mean a pointer to `std::any`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130330