[PATCH] D33533: [IR] Add an iterator and range accessor for the PHI nodes of a basic block.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/IR/BasicBlock.h:286
+
+    bool operator==(const phi_iterator_impl &Arg) const { return PN == Arg.PN; }
+
----------------
Generally prefer to use non-member op overloads where possible (but maybe the iterator_facade isn't compatible with this? Not sure). That way LHS and RHS conversions happen equally - relevant for, say, const+non-const_iterator comparisons.


================
Comment at: include/llvm/IR/BasicBlock.h:293
+      assert(PN && "Cannot increment the end iterator!");
+      PN = dyn_cast<PHINodeT>(std::next(BBIteratorT(PN)));
+      return *this;
----------------
Seems weird to build an iterator in every increment, though I imagine it's cheap - any reason to do this rather than to keep the iterator as a member (rather than the pointer as a member)?


================
Comment at: include/llvm/IR/BasicBlock.h:304-306
+  iterator_range<const_phi_iterator> phis() const {
+    return const_cast<BasicBlock *>(this)->phis();
+  }
----------------
Usually I expect to see this done the other way around - having the non-const version delegate to the const version. Otherwise the const version is technically UB when invoked on a const object (since it casts away constness, calls a function, etc). Though I realize that means needing an explicit conversion from const iterator to non-const iterator - maybe easier to have two implementations then, given it's short/simple.


================
Comment at: unittests/IR/BasicBlockTest.cpp:67
+  // And iterate a const range.
+  for (auto &PN : const_cast<const BasicBlock *>(BB.get())->phis()) {
+    EXPECT_EQ(BB.get(), PN.getIncomingBlock(0));
----------------
const_casts /to/ const kind of weird me out (because they're rare & so I'm only really used to seeing const_casts removing const - takes a bit to read when they're adding it). Should we have an llvm::implicit_cast?

Should it be "const auto &" on the LHS, if this is a const iterator?


https://reviews.llvm.org/D33533