[ADT] Add back ability to compare StringSet

[ArcsinX]

StringSet comparison was broken after moving from llvm::Optional to std::optional because std::nullopt_t is not equality-comparable. Without this patch a try to compare objects of StringSet type leads to compilation error:

llvm-project/llvm/include/llvm/ADT/StringMap.h:294:33: error: no match for ‘operator==’ (operand types are ‘std::nullopt_t’ and ‘std::nullopt_t’)
294 |       if (!(KeyValue.getValue() == FindInRHS->getValue()))

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Aleksandr Platonov (ArcsinX)

Changes

StringSet comparison was broken after moving from llvm::Optional to std::optional because std::nullopt_t is not equality-comparable. Without this patch a try to compare objects of StringSet type leads to compilation error:

llvm-project/llvm/include/llvm/ADT/StringMap.h:294:33: error: no match for ‘operator==’ (operand types are ‘std::nullopt_t’ and ‘std::nullopt_t’)
294 |       if (!(KeyValue.getValue() == FindInRHS->getValue()))

---

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

2 Files Affected:

• (modified) llvm/include/llvm/ADT/StringMap.h (+4-2)
• (modified) llvm/unittests/ADT/StringSetTest.cpp (+8)

diff --git a/llvm/include/llvm/ADT/StringMap.h b/llvm/include/llvm/ADT/StringMap.h
index 453d91349e3585..daaf82654e0948 100644
--- a/llvm/include/llvm/ADT/StringMap.h
+++ b/llvm/include/llvm/ADT/StringMap.h
@@ -291,8 +291,10 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
       if (FindInRHS == RHS.end())
         return false;
 
-      if (!(KeyValue.getValue() == FindInRHS->getValue()))
-        return false;
+      if constexpr (!std::is_same_v<ValueTy, std::nullopt_t>) {
+        if (!(KeyValue.getValue() == FindInRHS->getValue()))
+          return false;
+      }
     }
 
     return true;
diff --git a/llvm/unittests/ADT/StringSetTest.cpp b/llvm/unittests/ADT/StringSetTest.cpp
index e3703f6f01508b..a804c1f17d1ce2 100644
--- a/llvm/unittests/ADT/StringSetTest.cpp
+++ b/llvm/unittests/ADT/StringSetTest.cpp
@@ -73,4 +73,12 @@ TEST_F(StringSetTest, Contains) {
   EXPECT_FALSE(Set.contains("test"));
 }
 
+TEST_F(StringSetTest, Equal) {
+  StringSet<> A = {"A"};
+  StringSet<> B = {"B"};
+  ASSERT_TRUE(A != B);
+  ASSERT_FALSE(A == B);
+  ASSERT_TRUE(A == A);
+}
+
 } // end anonymous namespace

[dwblaikie]

Looks OK to me - bit of a weird special case for StringMap, so an alternative might be for StringSet to define its own, always-true equality comparable, value type to use like it did before. But I wouldn't insist on that.

[ArcsinX]

Looks OK to me - bit of a weird special case for StringMap, so an alternative might be for StringSet to define its own, always-true equality comparable, value type to use like it did before. But I wouldn't insist on that.

I thought about this, but it seems to me that it would be a bit of a step back. In such an approach we need to add a type similar to llvm::NoneType, which was previously removed as unnecessary.

[steakhal]

Thanks for the patch! I just wanted to fix this now.
FYI xref https://reviews.llvm.org/D138539#4164307