[PATCH] D18578: Introduce BitSet: A BitVector based class behaving like std::set/DenseSet

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

It is nice to have a separate `BitSet` instead of adding the API to the BitVector, but at the same time I wonder about some inefficiencies due the wrapping here? See inline for instance.


================
Comment at: include/llvm/ADT/BitSet.h:57
@@ +56,3 @@
+      advance();
+      return const_iterator(Vector, Prev);
+    }
----------------
This is requerying the Vector uselessly here (or post-increment is not good anyway but still).

================
Comment at: include/llvm/ADT/BitSet.h:64
@@ +63,3 @@
+      : Vector(Vector), E(E) {
+      advance();
+    }
----------------
Could the iterator be lazy and not do anything on creation? (that would solve the point above by the way)

================
Comment at: include/llvm/ADT/BitSet.h:111
@@ +110,3 @@
+    Vector[Idx] = true;
+    return std::make_pair(iterator(&Vector, Idx), Res);
+  }
----------------
Again that's 3 queries to the vector instead of one, I wonder if the compiler CSE perfectly the generated code. 


Repository:
  rL LLVM

http://reviews.llvm.org/D18578