[libc++] Make sure flat_{multi}map::key_compare handle boolean-testable correctly

[hewillk]

This is sibling of #69378.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Hewill Kang (hewillk)

Changes

This is sibling of #69378.

---

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

1 Files Affected:

• (modified) libcxx/include/__flat_map/flat_map.h (+2-2)

diff --git a/libcxx/include/__flat_map/flat_map.h b/libcxx/include/__flat_map/flat_map.h
index a0594ed9dc411..2f32497ab20d3 100644
--- a/libcxx/include/__flat_map/flat_map.h
+++ b/libcxx/include/__flat_map/flat_map.h
@@ -846,7 +846,7 @@ class flat_map {
         __compare_(std::forward<_CompArg>(__comp)...) {}
 
   _LIBCPP_HIDE_FROM_ABI bool __is_sorted_and_unique(auto&& __key_container) const {
-    auto __greater_or_equal_to = [this](const auto& __x, const auto& __y) { return !__compare_(__x, __y); };
+    auto __greater_or_equal_to = [this](const auto& __x, const auto& __y) -> bool { return !__compare_(__x, __y); };
     return ranges::adjacent_find(__key_container, __greater_or_equal_to) == ranges::end(__key_container);
   }
 
@@ -870,7 +870,7 @@ class flat_map {
       auto __zv                  = ranges::views::zip(__containers_.keys, __containers_.values);
       auto __append_start_offset = __containers_.keys.size() - __num_of_appended;
       auto __end                 = __zv.end();
-      auto __compare_key         = [this](const auto& __p1, const auto& __p2) {
+      auto __compare_key         = [this](const auto& __p1, const auto& __p2) -> bool {
         return __compare_(std::get<0>(__p1), std::get<0>(__p2));
       };
       if constexpr (!_WasSorted) {

[frederick-vs-ja]

I think it's expected that decltype(__compare_(__x, __y)) already model boolean-testable (or even is exactly bool in most normal usages).

If the standard were already so requiring, we wouldn't need to change these lambdas. But I haven't found such a requirement...

[frederick-vs-ja]

Oh, I was likely to be wrong. But are such conversions already performed in algorithms?

[hewillk]

Oh, I was likely to be wrong. But are such conversions already performed in algorithms?

https://godbolt.org/z/nbG9v4zqY

[frederick-vs-ja]

Oh, I see. It's the nature of lambda expression without trailing return type that unfortunately decay-copies the result.

Perhaps there should be some test cases for this, with types in boolean_testable.h reused?

[hewillk]

Perhaps there should be some test cases for this, with types in boolean_testable.h reused?

I don't have a local repository for llvm, co-working is welcome.

[frederick-vs-ja]

I recalled that the current standard wording doesn't actually require the result type of Predicate of erase_if for flat_meow to model boolean-testable. The boolean-testablity requirements seem only imposed for erase_if for sequence containers.

For flat_meow, the current wording uses explicit cast to bool. So should we use bool(__compare_(...)) instead, or open an LWG issue for this?

[hewillk]

I recalled that the current standard wording doesn't actually require the result type of Predicate of erase_if for flat_meow to model boolean-testable. The boolean-testablity requirements seem only imposed for erase_if for sequence containers.

I mainly referred to https://eel.is/c++draft/algorithms#requirements-6

[frederick-vs-ja]

I recalled that the current standard wording doesn't actually require the result type of Predicate of erase_if for flat_meow to model boolean-testable. The boolean-testablity requirements seem only imposed for erase_if for sequence containers.

I mainly referred to https://eel.is/c++draft/algorithms#requirements-6

I meant that such requirement doesn't seem properly propagated to erase_if for non-sequence containers and container adaptors. (And possibly hive?)

• For basic_string, deque, inplace_vector, and vector, the requirements are propagated via equivalent-to code that uses std::remove_if.
• For forward_list and list, the requirements are propagated via [forward.list.ops], [list.ops] and equivalent-to code that uses member remove_if.

But for other containers and container adaptors, there doesn't seem such requirement propagation. For hive it's quite weird that [hive.operations] propagates the requirements, but erase_if isn't in that subclause.

[hewillk]

But for other containers and container adaptors, there doesn't seem such requirement propagation. For hive it's quite weird that [hive.operations] propagates the requirements, but erase_if isn't in that subclause.

In any case, std::erase_if can be argued to be an algorithm.
"When not otherwise constrained, the Predicate parameter is used whenever an algorithm expects a function object..." should also apply IMHO.

[ldionne]

Can we add tests similarly to what we do in libcxx/test/std/algorithms/ranges_robust_against_nonbool.compile.pass.cpp?

[hewillk]

Can we add tests similarly to what we do in libcxx/test/std/algorithms/ranges_robust_against_nonbool.compile.pass.cpp?

No problem, I can handle this when I have the time .

[huixie90]

thanks for the patch. if you are not familiar with libc++ tests, i can certainly help with the testing for this patch. let me know if you want me to add the test to your branch

[ldionne]

@huixie90 Would you be able to make the testing changes here?

[huixie90]

Added Unit Tests to your branch

[frederick-vs-ja]

LGTM. I think we can merge this now.

Edit: Oops, I decided to change to use // REQUIRES: std-at-least-c++23 in another place. Let's waiting for CI becoming green.