[PATCH] D146136: [llvm][CycleInfo] Quick look-up for block in cycle.

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:73
+  using BlockSetVectorT =
+      SetVector<BlockT *, std::vector<BlockT *>, SmallPtrSet<BlockT *, 8>>;
+  BlockSetVectorT Blocks;
----------------
Maybe use `SmallVector<BlockT *, 8>` for the vector, to "match" the SmallPtrSet?


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:118-120
+    // We are forced to do a const_cast because our key_type is a pointer, and
+    // the "const" qualifier in SetVector::contains(const key_type&) gets
+    // applied to the wrong part of the type.
----------------
I don't think this explanation makes any sense. There is no world in which it would be "right" for the "const" in "const key_type&" to be applied to the pointee type of key_type.

We could add a templated `SetVector::contains` to avoid the need for the case, like `std::set` has: https://en.cppreference.com/w/cpp/container/set/contains


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146136