[ADT] Allow empty string in StringSet

[sbc100]

Summary

Also add a test case to wasm-ld that asserts without this change.
Internally wasm-ld builds a StringMap of exported functions and it seems
like allowing empty string in the set is preferable to adding checks.

This assert looks like it was most likely just a history accident.
It started life here purely to support InputLanguagesSet:

eeac27e38c5c567d63bbfa5410620d955696491b

Then got extracted here:

e57a4033385c5976cbb17af1e962b1224a61183b

Then got moved to ADT here

5c48bae209bcbd261886f63abac695b1e30544e6

With the InLang paramater name still intact which suggested is
InputLanguagesSet origins.

[dblaikie]

Probably best to test this functionality with a unit test for StringSet - but also please check if there's a reason StringSet might not support empty strings in its implementation in some way? (tombstone/other marker objects might be using "length 0" as a marker of some kind?)

[sbc100]

add test

[sbc100]

whitespace

[sbc100]

Added tests.

From inspection I can't see why this limitation would be needed. I also looked back though the history to identify the original reason for this limitation and it came, not from the data structure but from its domain-specific origin (a place where empty strings were not expected).

[dblaikie]

Sounds good - please commit the StringPool fix separately & I'd personally probably remove the wasm test, but I can see the merit in including it too, so up to you there.

I also looked back though the history to identify the original reason for this limitation and it came, not from the data structure but from its domain-specific origin (a place where empty strings were not expected).

Could you include a few of the details from the history there to explain it in the commit message?

[sbc100]

Will do

Could you include a few of the details from the history there to explain it in the commit message?

The change description goes into quite a bit of detail already no? Or am I missing something?

[sbc100]

Revert misc changes

[dblaikie]

In D74589#1947218, @sbc100 wrote:

Will do

Could you include a few of the details from the history there to explain it in the commit message?

The change description goes into quite a bit of detail already no? Or am I missing something?

ah, sure - that's quite sufficient - thanks!

llvm/include/llvm/ADT/StringSet.h

public:
     StringSet() = default;
     StringSet(std::initializer_list<StringRef> S) {
       for (StringRef X : S)
         insert(X);
     }
     explicit StringSet(AllocatorTy A) : base(A) {}
 
     std::pair<typename base::iterator, bool> insert(StringRef Key) {
-      assert(!Key.empty());
       return base::insert(std::make_pair(Key, None));
     }
 
     template <typename InputIt>
     void insert(const InputIt &Begin, const InputIt &End) {
       for (auto It = Begin; It != End; ++It)
         base::insert(std::make_pair(*It, None));
     }

llvm/unittests/ADT/StringSetTest.cpp

TEST_F(StringSetTest, InsertAndCountStringMapEntry) {
   StringMapEntry<StringRef> *Element = StringMapEntry<StringRef>::Create("A");
   Set.insert(*Element);
   size_t Count = Set.count(*Element);
   size_t Expected = 1;
   EXPECT_EQ(Expected, Count);
   Element->Destroy();
 }
 
+TEST_F(StringSetTest, EmptyString) {
+  // Verify that the empty string can by successfully inserted
+  StringSet<> Set;
+  size_t Count = Set.count("");
+  EXPECT_EQ(Count, 0UL);
+
+  Set.insert("");
+  Count = Set.count("");
+  EXPECT_EQ(Count, 1UL);
+}
+
 } // end anonymous namespace