[ADT] Use range-based helper functions in SmallSet

[vhscampos]

Replace code that relies on iterators by LLVM helper functions that take ranges. This makes the code simpler and more readable.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Victor Campos (vhscampos)

Changes

Replace code that relies on iterators by LLVM helper functions that take ranges. This makes the code simpler and more readable.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/ADT/SmallSet.h (+10-26)

diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index 630c98504261aa..d5f64e4f20f854 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -139,10 +139,6 @@ class SmallSet {
   SmallVector<T, N> Vector;
   std::set<T, C> Set;
 
-  using VIterator = typename SmallVector<T, N>::const_iterator;
-  using SIterator = typename std::set<T, C>::const_iterator;
-  using mutable_iterator = typename SmallVector<T, N>::iterator;
-
   // In small mode SmallPtrSet uses linear search for the elements, so it is
   // not a good idea to choose this value too high. You may consider using a
   // DenseSet<> instead if you expect many elements in the set.
@@ -163,13 +159,7 @@ class SmallSet {
   }
 
   /// count - Return 1 if the element is in the set, 0 otherwise.
-  size_type count(const T &V) const {
-    if (isSmall()) {
-      // Since the collection is small, just do a linear search.
-      return vfind(V) == Vector.end() ? 0 : 1;
-    }
-    return Set.count(V);
-  }
+  size_type count(const T &V) const { return contains(V) ? 1 : 0; }
 
   /// insert - Insert an element into the set if it isn't already there.
   /// Returns a pair. The first value of it is an iterator to the inserted
@@ -181,7 +171,7 @@ class SmallSet {
       return std::make_pair(const_iterator(I), Inserted);
     }
 
-    VIterator I = vfind(V);
+    auto I = llvm::find(Vector, V);
     if (I != Vector.end())    // Don't reinsert if it already exists.
       return std::make_pair(const_iterator(I), false);
     if (Vector.size() < N) {
@@ -206,11 +196,12 @@ class SmallSet {
   bool erase(const T &V) {
     if (!isSmall())
       return Set.erase(V);
-    for (mutable_iterator I = Vector.begin(), E = Vector.end(); I != E; ++I)
-      if (*I == V) {
-        Vector.erase(I);
-        return true;
-      }
+
+    auto It = llvm::find(Vector, V);
+    if (It != Vector.end()) {
+      Vector.erase(It);
+      return true;
+    }
     return false;
   }
 
@@ -234,19 +225,12 @@ class SmallSet {
   /// Check if the SmallSet contains the given element.
   bool contains(const T &V) const {
     if (isSmall())
-      return vfind(V) != Vector.end();
-    return Set.find(V) != Set.end();
+      return llvm::is_contained(Vector, V);
+    return llvm::is_contained(Set, V);
   }
 
 private:
   bool isSmall() const { return Set.empty(); }
-
-  VIterator vfind(const T &V) const {
-    for (VIterator I = Vector.begin(), E = Vector.end(); I != E; ++I)
-      if (*I == V)
-        return I;
-    return Vector.end();
-  }
 };
 
 /// If this set is of pointer values, transparently switch over to using

[kuhar]

Nice

[kuhar]

nit: this is not used outside of the if

Suggested change

	auto It = llvm::find(Vector, V);
	if (It != Vector.end()) {
	if (auto It = llvm::find(Vector, V); It != Vector.end()) {

[s-barannikov]

I'm slightly opposed to this change because it pulls in STLExtras.h.
Yes, it is already included, but it would be better to avoid including it.

[nikic]

Please do not use is_contained with types that do not require linear scan. This should be return Set.contains(V).

[vhscampos]

Thanks for pointing that out. I've checked and llvm::is_contained indeed does a linear scan on a std::set because the latter does not have a contains method before C++20.

[kuhar]

I'd expect it to call set::find:

llvm-project/llvm/include/llvm/ADT/STLExtras.h

Lines 1890 to 1891 in 49a754a

	else if constexpr (detail::HasMemberFind<R, E>)
	return Range.find(Element) != Range.end();

[vhscampos]

My bad. @kuhar is right. It calls std::set::find which is as good as it gets with std::set.

[nikic]

It doesn't matter what it actually calls, we should never use is_contained() for set structures, unless there is some very strong reason (e.g. generic context that really does not care whether it gets an O(1) set or O(n) set).

[dwblaikie]

This advice seems at least inconsistent with the intent of the code per the comment:

This is intended as the canonical way to check if an
element exists in a range in generic code or range type that does not
expose a `.contains(Element)` member.

In this case, the set meets the second requirement - that it does not expose a contains function? (in C++17, at least - it is in C++20, but LLVM isn't using that yet)

[nikic]

If you want to have an abstraction over contains() and find() != end(), I'd suggest adding another helper, which does not also include the linear scan case. Though I don't think this is really worthwhile, as the existing find() != end() code works fine.

[dwblaikie]

FWIW, I think is_contained is more legible than find!=end and seems fine to use here & I agree with the comment on is_contained suggesting it for uses like this.

[vhscampos]

I'm slightly opposed to this change because it pulls in STLExtras.h. Yes, it is already included, but it would be better to avoid including it.

Why is including STLExtras.h a problem?

[vhscampos]

Given that this change isn't exactly agreeable, I plan to change it to a PR to make SmallSet not use STLExtras.h at all.

[vhscampos]

New PR: #109412

[kuhar]

Closing based on #108585 (comment)