[PATCH] D74984: [ADT] Add CoalescingBitVector, implemented using IntervalMap [1/3]

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Is there any kind of "These are the ADT types in LLVM and when to use them" .rst document that this should be added to?



================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:29
+/// Compared to SparseBitVector, CoalescingBitVector offers more predictable
+/// performance for non-sequential find() operations.
+template <typename EltT, unsigned N = 16> class CoalescingBitVector {
----------------
Given that this is a bitvector, I'm kind of wondering what EltT is supposed to be (bool?) Can you add that to the class comment?


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:38
+
+  using IntervalT = std::pair<EltT, EltT>;
+
----------------
Ah.. it's the *Index* type? Perhaps that's a better name?


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:79
+  /// Count the number of set bits.
+  unsigned count() const {
+    unsigned Bits = 0;
----------------
Is this consistent with how the other bitvector types are naming this function? Then that's fine. In DenseSet, count(Elt) is a contains(Elt) function and I don't know if we want to be consistent with that.


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:82
+    for (auto It = Intervals->begin(), End = Intervals->end(); It != End; ++It)
+      Bits += 1 + It.stop() - It.start();
+    return Bits;
----------------
So empty ranges are disallowed? Should we assert that? Or is that too slow?


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:142
+
+  /// Reset all bits present in \p RHS.
+  bool intersectWithComplement(const ThisT &RHS) {
----------------
document \return


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:297
+  void print(raw_ostream &OS) const {
+    // LLDB swallows the first line of output after callling dump(). Add
+    // newlines before/after the braces to work around this.
----------------
Really? If anything should this be part of the dump method instead?


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

https://reviews.llvm.org/D74984