Page MenuHomePhabricator

[C++11] Add predecessors(BasicBlock *) / successors(BasicBlock *) iterator ranges.
ClosedPublic

Authored by mjacob on Jul 12 2014, 8:06 AM.

Details

Summary

This patch introduces two new iterator ranges and updates existing code to use it. No functional change intended.

Diff Detail

Event Timeline

mjacob updated this revision to Diff 11336.Jul 12 2014, 8:06 AM
mjacob retitled this revision from to [C++11] Add predecessors(BasicBlock *) / successors(BasicBlock *) iterator ranges..
mjacob updated this object.
mjacob edited the test plan for this revision. (Show Details)
mjacob added a subscriber: Unknown Object (MLST).
mjacob updated this revision to Diff 11405.Jul 14 2014, 2:04 PM

Update patch to make it apply cleanly to HEAD.

dblaikie edited edge metadata.Jul 19 2014, 11:14 AM

Generally looks good - some optional comments, but they're potentially orthogonal/follow-up work.

lib/Analysis/BranchProbabilityInfo.cpp
533–534

You could drop the {} from this loop (and the outer loop, actually... ), but I don't think it's necessary in this cleanup (and may even be better as a separate commit anyway)

533–534

I think we've been pretty forward about using "auto" more aggressively - especially in "obvious" cases, which I think this qualifies (BI might one day be "BB" when that outer loop is changed to a range-based-for, but either name connotes "basic block" pretty pervasively in LLVM - and iterating over the predecessors or successors of a basic block clearly consists of more basic blocks).

So maybe these should be "auto *" and "const auto *" throughout the refactoring.

mjacob added inline comments.Jul 19 2014, 2:31 PM
lib/Analysis/BranchProbabilityInfo.cpp
533–534

Yes, I'll do these changes (rangify outer loop, remove braces) after committing this one.

I'm not sure if it's better to use "auto" here -- it doesn't increase readability. I don't think "BasicBlock" is a very cumbersome name. "Succ" is a general name that could also mean "successor instruction" for example.

dblaikie accepted this revision.Jul 19 2014, 9:19 PM
dblaikie edited edge metadata.

LGTM

(In follow up many of these could be std::one_of, std::any_of, none_of, etc - though we could add llvm versions of those that work on ranges directly (rather than iterator begin/end))

This revision is now accepted and ready to land.Jul 19 2014, 9:19 PM
mjacob closed this revision.Jul 20 2014, 2:18 AM