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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks for all the comments Dave! I'm gonna land this with the fix below, but do let me know if you want any further work here.



================
Comment at: include/llvm/IR/BasicBlock.h:304-306
+  iterator_range<const_phi_iterator> phis() const {
+    return const_cast<BasicBlock *>(this)->phis();
+  }
----------------
dblaikie wrote:
> chandlerc wrote:
> > 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...)
> Fair - can't find anything that says it's UB to call a non-const function on a const object, but weirds me out a bit because it'd make it pretty easy to leak a non-const pointer/mutate state by accident, so I'd tend to prefer the inverse phrasing, but I realize it's a bit awkward. Up to you.
Digging around, we do this ... pretty frequently. And I dunno that having an explicit conversion from a const_iterator to non-const iterator is any less weird. In both cases, we're stripping off const and doing something with it.

Anyways, I'm gonna leave this as-is for now as it doesn't seem uncommon. Happy to revisit this if you and/or others come the consensus that we really want to shift away from this.


================
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:
> chandlerc wrote:
> > 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...
> I'd love an implicit_cast - if you want to add one while you're at it, but I understand that's a whole nother thing with test cases, etc.
> 
> I figure const, &, and * are short enough that the relative cost/benefit is worthwhile to include them. The LLVM style guide does at least call for '*' when auto is deducing to a pointer (/when that matters - if it's some opaque typedef that the code doesn't care about pointerness, I suppose '*' wouldn't be used) & I've always assumed (incorrectly, now that I look) that the style guide said the same about const. At least that's my habit.
> 
> Up to you.
> 
Adding const here.

Happy for this to switch to an implicit cast if/when one shows up, but I'm gonna resist shaving that yak today. ;]


https://reviews.llvm.org/D33533