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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked an inline comment as done.
sameerds added inline comments.


================
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.
----------------
foad wrote:
> 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
Actually it's not about what the const is applied to. The simple fact is that when you lookup in a container, the argument has to match the key_type. The same compilation error happens with an std::set<Foo*> where you try to call set::count() with a const Foo* argument.

I think I followed the whole thing through to the most appropriate fix, which is to recognize that sometimes, the user of a SetVector wants a vector of Foo* but a set of const Foo*. Updated the SetVector to make that happen. Again, this expectation is identical to LoopInfo.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:181
   //@{
-  using const_block_iterator = typename std::vector<BlockT *>::const_iterator;
+  using const_block_iterator = typename BlockSetVectorT::const_iterator;
 
----------------
Pierre-vh wrote:
> Curious: Do you still need typename here?
Yes, because BlockSetVectorT is a dependent type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146136