[llvm] [ADT] Series of improvements to SmallSet (PR #104611)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 16 09:23:21 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-adt
Author: Victor Campos (vhscampos)
<details>
<summary>Changes</summary>
This PR introduces a series of improvements to the SmallSet data structure.
- Use universal reference/perfect forwarding in `SmallSet::insert` interface.
- Introduction of several new API functions:
- Constructor that takes pair of iterators.
- Constructor that takes a range.
- Constructor that takes an initializer list.
- Copy constructor.
- Move constructor.
- Copy assignment operator.
- Move assignment operator.
- Style warnings fixes.
- Modernisation of code using newer C++ constructs.
---
Full diff: https://github.com/llvm/llvm-project/pull/104611.diff
2 Files Affected:
- (modified) llvm/include/llvm/ADT/SmallSet.h (+73-66)
- (modified) llvm/unittests/ADT/SmallSetTest.cpp (+60)
``````````diff
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index a16e8ac6f07552..7898b7b11eb212 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -14,14 +14,13 @@
#ifndef LLVM_ADT_SMALLSET_H
#define LLVM_ADT_SMALLSET_H
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/iterator.h"
-#include "llvm/Support/Compiler.h"
-#include "llvm/Support/type_traits.h"
#include <cstddef>
#include <functional>
+#include <initializer_list>
#include <set>
#include <type_traits>
#include <utility>
@@ -46,24 +45,24 @@ class SmallSetIterator
VecIterTy VecIter;
};
- bool isSmall;
+ bool IsSmall;
public:
- SmallSetIterator(SetIterTy SetIter) : SetIter(SetIter), isSmall(false) {}
+ SmallSetIterator(SetIterTy SetIter) : SetIter(SetIter), IsSmall(false) {}
- SmallSetIterator(VecIterTy VecIter) : VecIter(VecIter), isSmall(true) {}
+ SmallSetIterator(VecIterTy VecIter) : VecIter(VecIter), IsSmall(true) {}
// Spell out destructor, copy/move constructor and assignment operators for
// MSVC STL, where set<T>::const_iterator is not trivially copy constructible.
~SmallSetIterator() {
- if (isSmall)
+ if (IsSmall)
VecIter.~VecIterTy();
else
SetIter.~SetIterTy();
}
- SmallSetIterator(const SmallSetIterator &Other) : isSmall(Other.isSmall) {
- if (isSmall)
+ SmallSetIterator(const SmallSetIterator &Other) : IsSmall(Other.IsSmall) {
+ if (IsSmall)
VecIter = Other.VecIter;
else
// Use placement new, to make sure SetIter is properly constructed, even
@@ -71,8 +70,8 @@ class SmallSetIterator
new (&SetIter) SetIterTy(Other.SetIter);
}
- SmallSetIterator(SmallSetIterator &&Other) : isSmall(Other.isSmall) {
- if (isSmall)
+ SmallSetIterator(SmallSetIterator &&Other) : IsSmall(Other.IsSmall) {
+ if (IsSmall)
VecIter = std::move(Other.VecIter);
else
// Use placement new, to make sure SetIter is properly constructed, even
@@ -83,11 +82,11 @@ class SmallSetIterator
SmallSetIterator& operator=(const SmallSetIterator& Other) {
// Call destructor for SetIter, so it gets properly destroyed if it is
// not trivially destructible in case we are setting VecIter.
- if (!isSmall)
+ if (!IsSmall)
SetIter.~SetIterTy();
- isSmall = Other.isSmall;
- if (isSmall)
+ IsSmall = Other.IsSmall;
+ if (IsSmall)
VecIter = Other.VecIter;
else
new (&SetIter) SetIterTy(Other.SetIter);
@@ -97,11 +96,11 @@ class SmallSetIterator
SmallSetIterator& operator=(SmallSetIterator&& Other) {
// Call destructor for SetIter, so it gets properly destroyed if it is
// not trivially destructible in case we are setting VecIter.
- if (!isSmall)
+ if (!IsSmall)
SetIter.~SetIterTy();
- isSmall = Other.isSmall;
- if (isSmall)
+ IsSmall = Other.IsSmall;
+ if (IsSmall)
VecIter = std::move(Other.VecIter);
else
new (&SetIter) SetIterTy(std::move(Other.SetIter));
@@ -109,22 +108,22 @@ class SmallSetIterator
}
bool operator==(const SmallSetIterator &RHS) const {
- if (isSmall != RHS.isSmall)
+ if (IsSmall != RHS.IsSmall)
return false;
- if (isSmall)
+ if (IsSmall)
return VecIter == RHS.VecIter;
return SetIter == RHS.SetIter;
}
SmallSetIterator &operator++() { // Preincrement
- if (isSmall)
- VecIter++;
+ if (IsSmall)
+ ++VecIter;
else
- SetIter++;
+ ++SetIter;
return *this;
}
- const T &operator*() const { return isSmall ? *VecIter : *SetIter; }
+ const T &operator*() const { return IsSmall ? *VecIter : *SetIter; }
};
/// SmallSet - This maintains a set of unique values, optimizing for the case
@@ -139,10 +138,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.
@@ -155,6 +150,22 @@ class SmallSet {
using const_iterator = SmallSetIterator<T, N, C>;
SmallSet() = default;
+ SmallSet(const SmallSet &) = default;
+ SmallSet(SmallSet &&) = default;
+
+ template <typename IterT> explicit SmallSet(IterT Begin, IterT End) {
+ this->insert(Begin, End);
+ }
+
+ template <typename RangeT>
+ explicit SmallSet(const iterator_range<RangeT> &R) {
+ this->insert(R.begin(), R.end());
+ }
+
+ SmallSet(std::initializer_list<T> L) { this->insert(L.begin(), L.end()); }
+
+ SmallSet &operator=(const SmallSet &) = default;
+ SmallSet &operator=(SmallSet &&) = default;
[[nodiscard]] bool empty() const { return Vector.empty() && Set.empty(); }
@@ -163,39 +174,16 @@ 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;
- } else {
- 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
/// element or the existing element in the set. The second value is true
/// if the element is inserted (it was not in the set before).
- std::pair<const_iterator, bool> insert(const T &V) {
- if (!isSmall()) {
- auto [I, Inserted] = Set.insert(V);
- return std::make_pair(const_iterator(I), Inserted);
- }
-
- VIterator I = vfind(V);
- if (I != Vector.end()) // Don't reinsert if it already exists.
- return std::make_pair(const_iterator(I), false);
- if (Vector.size() < N) {
- Vector.push_back(V);
- return std::make_pair(const_iterator(std::prev(Vector.end())), true);
- }
+ std::pair<const_iterator, bool> insert(const T &V) { return insertImpl(V); }
- // Otherwise, grow from vector to set.
- while (!Vector.empty()) {
- Set.insert(Vector.back());
- Vector.pop_back();
- }
- return std::make_pair(const_iterator(Set.insert(V).first), true);
+ std::pair<const_iterator, bool> insert(T &&V) {
+ return insertImpl(std::move(V));
}
template <typename IterT>
@@ -207,11 +195,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;
}
@@ -235,18 +224,36 @@ 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();
+ template <typename ArgType>
+ std::pair<const_iterator, bool> insertImpl(ArgType &&V) {
+ static_assert(std::is_convertible_v<ArgType, T>,
+ "ArgType must be convertible to T!");
+ if (!isSmall()) {
+ auto [I, Inserted] = Set.insert(std::forward<ArgType>(V));
+ return std::make_pair(const_iterator(I), Inserted);
+ }
+
+ 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) {
+ Vector.push_back(std::forward<ArgType>(V));
+ return std::make_pair(const_iterator(std::prev(Vector.end())), true);
+ }
+
+ // Otherwise, grow from vector to set.
+ Set.insert(std::make_move_iterator(Vector.begin()),
+ std::make_move_iterator(Vector.end()));
+ Vector.clear();
+ return std::make_pair(
+ const_iterator(Set.insert(std::forward<ArgType>(V)).first), true);
}
};
diff --git a/llvm/unittests/ADT/SmallSetTest.cpp b/llvm/unittests/ADT/SmallSetTest.cpp
index b50b368ae66361..ced1ba5dce34d8 100644
--- a/llvm/unittests/ADT/SmallSetTest.cpp
+++ b/llvm/unittests/ADT/SmallSetTest.cpp
@@ -17,6 +17,66 @@
using namespace llvm;
+TEST(SmallSetTest, ConstructorIteratorPair) {
+ auto L = {1, 2, 3, 4, 5};
+ SmallSet<int, 4> S(std::begin(L), std::end(L));
+ for (int Value : L)
+ EXPECT_TRUE(S.contains(Value));
+}
+
+TEST(SmallSet, ConstructorRange) {
+ auto L = {1, 2, 3, 4, 5};
+
+ SmallSet<int, 4> S(llvm::make_range(std::begin(L), std::end(L)));
+ for (int Value : L)
+ EXPECT_TRUE(S.contains(Value));
+}
+
+TEST(SmallSet, ConstructorInitializerList) {
+ auto L = {1, 2, 3, 4, 5};
+ SmallSet<int, 4> S = {1, 2, 3, 4, 5};
+ for (int Value : L)
+ EXPECT_TRUE(S.contains(Value));
+}
+
+TEST(SmallSet, CopyConstructor) {
+ SmallSet<int, 4> S = {1, 2, 3};
+ SmallSet<int, 4> T = S;
+
+ EXPECT_EQ(S, T);
+}
+
+TEST(SmallSet, MoveConstructor) {
+ auto L = {1, 2, 3};
+ SmallSet<int, 4> S = L;
+ SmallSet<int, 4> T = std::move(S);
+
+ EXPECT_TRUE(T.size() == L.size());
+ for (int Value : L) {
+ EXPECT_TRUE(T.contains(Value));
+ }
+}
+
+TEST(SmallSet, CopyAssignment) {
+ SmallSet<int, 4> S = {1, 2, 3};
+ SmallSet<int, 4> T;
+ T = S;
+
+ EXPECT_EQ(S, T);
+}
+
+TEST(SmallSet, MoveAssignment) {
+ auto L = {1, 2, 3};
+ SmallSet<int, 4> S = L;
+ SmallSet<int, 4> T;
+ T = std::move(S);
+
+ EXPECT_TRUE(T.size() == L.size());
+ for (int Value : L) {
+ EXPECT_TRUE(T.contains(Value));
+ }
+}
+
TEST(SmallSetTest, Insert) {
SmallSet<int, 4> s1;
``````````
</details>
https://github.com/llvm/llvm-project/pull/104611
More information about the llvm-commits
mailing list