[PATCH] D152497: [SetVector] Implement better "small" functionality

[Dhruv Chawla via Phabricator via llvm-commits]

0xdc03 added inline comments.
Herald added a subscriber: jdoerfert.


================
Comment at: llvm/include/llvm/ADT/SetVector.h:50
+          typename Set = DenseSet<T>, unsigned N = 0>
 class SetVector {
 public:
----------------
nikic wrote:
> It might make sense to replicate this static_assert from SmallPtrSet: https://github.com/llvm/llvm-project/blob/ecbd37d5a336ec8a8adafae5a8e4263bb738718c/llvm/include/llvm/ADT/SmallPtrSet.h#L451-L454
> 
> To make sure people don't shoot themselves in the foot with a nice `SmallSetVector<T, 512>`.
> 
> From a quick grep, this is the only usage that would have to be adjusted: https://github.com/llvm/llvm-project/blob/ecbd37d5a336ec8a8adafae5a8e4263bb738718c/flang/lib/Lower/Bridge.cpp#L190
Done, though I'm unsure if its better to have a higher limit of N (for example `SmallVector` has no limits).


================
Comment at: llvm/include/llvm/ADT/SetVector.h:202-209
+    if constexpr (canBeSmall())
+      if (isSmall())
+        return vector_.erase(I);
+
     const key_type &V = *I;
     assert(set_.count(V) && "Corrupted SetVector instances!");
     set_.erase(V);
----------------
nikic wrote:
> Could write it like this, not sure if better.
I do believe this would change the logic of the operation, because this would allow an empty set but non-empty vector to work as a well-defined `SetVector` when not in the small case while erasing. I think the correct check would be something like `(canBeSmall() && isSmall()) || set_.count(V)`, which is also fine I guess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152497/new/

https://reviews.llvm.org/D152497