[libc++] Make sure ranges algorithms and views handle boolean-testable correctly

[ldionne]

Before this patch, we would fail to implicitly convert the result of
predicates to bool, which means we'd potentially perform a copy or move
construction of the boolean-testable, which isn't allowed. The same
holds true for comparing iterators against sentinels, which is allowed
to return a boolean-testable type.

We already had tests aiming to ensure correct handling of these types,
but they failed to provide appropriate coverage in several cases due to
guaranteed RVO. This patch fixes the tests, adds tests for missing
algorithms and views, and fixes the actual problems in the code.

Fixes #69074

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

We would fail to implicitly convert the result of the predicate to bool, which means we'd potentially perform a copy or move construction of the boolean-testable, which isn't allowed. We already had tests aiming to ensure correct handling of these types, but they failed to catch copy and move construction because of guaranteed RVO.

Fixes #69074

---

Full diff: https://github.com/llvm/llvm-project/pull/69378.diff

3 Files Affected:

• (modified) libcxx/include/__algorithm/ranges_find_if_not.h (+2-2)
• (modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if_not.pass.cpp (+1-1)
• (modified) libcxx/test/support/boolean_testable.h (+6-3)

diff --git a/libcxx/include/__algorithm/ranges_find_if_not.h b/libcxx/include/__algorithm/ranges_find_if_not.h
index 6beade1462e099c..a18bea43165e0d8 100644
--- a/libcxx/include/__algorithm/ranges_find_if_not.h
+++ b/libcxx/include/__algorithm/ranges_find_if_not.h
@@ -39,14 +39,14 @@ struct __fn {
             indirect_unary_predicate<projected<_Ip, _Proj>> _Pred>
   _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _Ip
   operator()(_Ip __first, _Sp __last, _Pred __pred, _Proj __proj = {}) const {
-    auto __pred2 = [&](auto&& __e) { return !std::invoke(__pred, std::forward<decltype(__e)>(__e)); };
+    auto __pred2 = [&](auto&& __e) -> bool { return !std::invoke(__pred, std::forward<decltype(__e)>(__e)); };
     return ranges::__find_if_impl(std::move(__first), std::move(__last), __pred2, __proj);
   }
 
   template <input_range _Rp, class _Proj = identity, indirect_unary_predicate<projected<iterator_t<_Rp>, _Proj>> _Pred>
   _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr borrowed_iterator_t<_Rp>
   operator()(_Rp&& __r, _Pred __pred, _Proj __proj = {}) const {
-    auto __pred2 = [&](auto&& __e) { return !std::invoke(__pred, std::forward<decltype(__e)>(__e)); };
+    auto __pred2 = [&](auto&& __e) -> bool { return !std::invoke(__pred, std::forward<decltype(__e)>(__e)); };
     return ranges::__find_if_impl(ranges::begin(__r), ranges::end(__r), __pred2, __proj);
   }
 };
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if_not.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if_not.pass.cpp
index 95860745f56204e..03d43ebb752bff2 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if_not.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if_not.pass.cpp
@@ -227,7 +227,7 @@ constexpr bool test() {
     }
     {
       int a[] = {1, 2, 3, 4};
-      auto ret = std::ranges::find_if_not(a, [](const int& b) { return BooleanTestable{b != 3}; });
+      auto ret = std::ranges::find_if_not(a, [](const int& i) { return BooleanTestable{i != 3}; });
       assert(ret == a + 2);
     }
   }
diff --git a/libcxx/test/support/boolean_testable.h b/libcxx/test/support/boolean_testable.h
index e810e4e0461dc69..22bcefe04f9a53c 100644
--- a/libcxx/test/support/boolean_testable.h
+++ b/libcxx/test/support/boolean_testable.h
@@ -11,6 +11,8 @@
 
 #include "test_macros.h"
 
+#include <utility>
+
 #if TEST_STD_VER > 17
 
 class BooleanTestable {
@@ -24,11 +26,12 @@ class BooleanTestable {
   }
 
   friend constexpr BooleanTestable operator!=(const BooleanTestable& lhs, const BooleanTestable& rhs) {
-    return !(lhs == rhs);
+    return lhs.value_ != rhs.value_;
   }
 
-  constexpr BooleanTestable operator!() {
-    return BooleanTestable{!value_};
+  constexpr BooleanTestable&& operator!() && {
+    value_ = !value_;
+    return std::move(*this);
   }
 
   // this class should behave like a bool, so the constructor shouldn't be explicit

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ldionne]

The reporter of this bug looked at the PR and said it looked exhaustive to them: #69074 (comment).

Merging to finally close this and since there seems to be little interest from other people to do an in-depth review, but I'll welcome any post-commit comments.