[libc++] Remove the constexpr hash<vector<bool>> extension

[frederick-vs-ja]

libc++ makes the hash<vector<bool, A>>::operator() constexpr since C++20, which is a conforming extension, but it was unintended.

This patch removes the extension, with an escape hatch macro for it, and the escape hatch will be removed in the future. Test cases for constexpr along with the assumption of hash values are moved to the libcxx/test/libcxx/ subdirectory.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

libc++ makes the hash<vector<bool, A>>::operator() constexpr since C++20, which is a conforming extension. This patch move the test cases for constexpr along with the assumption of hash values to the libcxx/test/libcxx/ subdirectory.

---

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

3 Files Affected:

• (added) libcxx/test/libcxx/containers/sequences/vector.bool/hash.constexpr.pass.cpp (+53)
• (modified) libcxx/test/std/containers/sequences/vector.bool/enabled_hash.pass.cpp (+1-6)
• (modified) libcxx/test/std/containers/sequences/vector.bool/vector_bool.pass.cpp (+6-4)

diff --git a/libcxx/test/libcxx/containers/sequences/vector.bool/hash.constexpr.pass.cpp b/libcxx/test/libcxx/containers/sequences/vector.bool/hash.constexpr.pass.cpp
new file mode 100644
index 0000000000000..49874d0504e8a
--- /dev/null
+++ b/libcxx/test/libcxx/containers/sequences/vector.bool/hash.constexpr.pass.cpp
@@ -0,0 +1,53 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: std-at-least-c++20
+
+// <vector>
+
+// template<class Allocator> struct hash<vector<bool, Allocator>>;
+
+// libc++ makes the operator() of this partial specialization constexpr since C++20, which is a conforming extension.
+
+#include <cassert>
+#include <vector>
+
+#include "min_allocator.h"
+#include "poisoned_hash_helper.h"
+
+constexpr bool test() {
+  {
+    using VB = std::vector<bool>;
+    bool ba[]{true, false, true, true, false};
+    VB vb(std::begin(ba), std::end(ba));
+
+    const std::hash<VB> h{};
+    const auto hash_value = h(vb);
+    assert(hash_value == h(vb));
+    assert(hash_value != 0);
+  }
+  {
+    using VB  = std::vector<bool, min_allocator<bool>>;
+    bool ba[] = {true, false, true, true, false};
+    VB vb(std::begin(ba), std::end(ba));
+
+    const std::hash<VB> h{};
+    const auto hash_value = h(vb);
+    assert(hash_value == h(vb));
+    assert(hash_value != 0);
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}
diff --git a/libcxx/test/std/containers/sequences/vector.bool/enabled_hash.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/enabled_hash.pass.cpp
index 41cedd68fe50e..cba3101ef5009 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/enabled_hash.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/enabled_hash.pass.cpp
@@ -19,19 +19,14 @@
 #include "test_macros.h"
 #include "min_allocator.h"
 
-TEST_CONSTEXPR_CXX20 bool test() {
+void test() {
   test_hash_enabled<std::vector<bool> >();
   test_hash_enabled<std::vector<bool, min_allocator<bool>>>();
-
-  return true;
 }
 
 int main(int, char**) {
   test_library_hash_specializations_available();
   test();
-#if TEST_STD_VER > 17
-  static_assert(test());
-#endif
 
   return 0;
 }
diff --git a/libcxx/test/std/containers/sequences/vector.bool/vector_bool.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/vector_bool.pass.cpp
index e270869a8320f..f6bdb7fd5b3ef 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/vector_bool.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/vector_bool.pass.cpp
@@ -14,8 +14,6 @@
 //     size_t operator()(T val) const;
 // };
 
-// Not very portable
-
 #include <vector>
 #include <cassert>
 #include <iterator>
@@ -37,7 +35,9 @@ TEST_CONSTEXPR_CXX20 bool tests() {
     bool ba[] = {true, false, true, true, false};
     T vb(std::begin(ba), std::end(ba));
     H h;
-    assert(h(vb) != 0);
+    if (!TEST_IS_CONSTANT_EVALUATED) {
+      assert(h(vb) == h(vb));
+    }
   }
 #if TEST_STD_VER >= 11
   {
@@ -51,7 +51,9 @@ TEST_CONSTEXPR_CXX20 bool tests() {
     bool ba[] = {true, false, true, true, false};
     T vb(std::begin(ba), std::end(ba));
     H h;
-    assert(h(vb) != 0);
+    if (!TEST_IS_CONSTANT_EVALUATED) {
+      assert(h(vb) == h(vb));
+    }
   }
 #endif
 

[philnik777]

More generally, do we actually want to have this extension? (Assuming I added it when I made vector constexpr) I didn't decide to make this an extension consciously. Given that there is so far no standard feature that requires a constexpr hasher it's unlikely anybody is depending on it.

[ldionne]

IMO we should remove that extension. If it was added as a mistake, we should remove it.

It's probably best to make it non-constexpr but provide a macro to make it constexpr again for one release, just to provide a migration path. That would be my preferred direction here.

[frederick-vs-ja]

@philnik777 @ldionne Now I'm switching to remove the extension.

[philnik777]

LGTM, but let's wait for Louis as well.