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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

This is a follow-up to https://reviews.llvm.org/D130197 and https://reviews.llvm.org/D130212. I have manually confirmed that these tests would have found the bug fixed by those two patches.



================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:9
+
+// UNSUPPORTED: c++03
+
----------------
Making this support C++03 would be pretty painful because it doesn't support initializer lists.


================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:462
+template <class Iter>
+TEST_CONSTEXPR_CXX20 std::array<Input<typename Iter::value_type>, 8> get_simple_in() {
+  using T = typename Iter::value_type;
----------------
There needs to be a compromise between providing valid inputs for each algorithm and having a separate input for every single function. On one hand, the inputs have to satisfy the preconditions (or else we'd trigger undefined behavior) and exercise many (ideally all) the code paths in the algorithm (because issues are only found if they're triggered). On the other hand, having separate inputs for each algorithm is unmaintainable. The idea here is to split all algorithms into a few groups and provide a set of interesting inputs for each group (e.g. the default set, the sorted set, the partitioned set, etc.).


================
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); });
----------------
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.


================
Comment at: libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp:557
+  test(simple_in, [&](I b, I e) { std::find_if_not(b, e, is_neg); });
+  // TODO: find_first_of
+  test(simple_in, [&](I b, I e) { std::adjacent_find(b, e); });
----------------
I know it's a lot of TODOs, but I'd like to get some feedback before spending time on how to test algorithms with more complicated inputs. This patch already improves coverage significantly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130330