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

Chandler Carruth via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 24 20:12:37 PDT 2017


chandlerc added inline comments.


================
Comment at: include/llvm/IR/BasicBlock.h:286
+
+    bool operator==(const phi_iterator_impl &Arg) const { return PN == Arg.PN; }
+
----------------
dblaikie wrote:
> 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.
I have no idea, but all the uses of iterator_facade_base follow this pattern. I'd somewhat rather not try to fix this in this patch....


================
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;
----------------
dblaikie wrote:
> 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)?
It avoids the dyn_cast on every dereference (or having more state).


================
Comment at: include/llvm/IR/BasicBlock.h:304-306
+  iterator_range<const_phi_iterator> phis() const {
+    return const_cast<BasicBlock *>(this)->phis();
+  }
----------------
dblaikie wrote:
> 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.
Why is it UB to cast away const even on a const object? I don't think anything makes that UB.

The UB thing is to *modify* a const object. But the non-const function here doesn't do that, so it seems fine?

I can have two implementations, but then I have to put them both out-of-line because the implementation of this method cannot be written in this header (requires the definition of the PHINode type which requires the definition of the BasicBlock type...)


================
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));
----------------
dblaikie wrote:
> 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?
To the first point: what would you suggest I write? I don't really care, this seemed to work.

To the second point: why? auto will deduce const just fine... adding const myself won't really surface any bugs. I guess I always prefer to write less when there is no reason to write more...


https://reviews.llvm.org/D33533





More information about the llvm-commits mailing list