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

David Blaikie dblaikie at gmail.com
Sat Jul 19 11:14:52 PDT 2014


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

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:533
@@ -532,4 +532,3 @@
        BI != BE; ++BI) {
-    for (succ_const_iterator SI = succ_begin(BI), SE = succ_end(BI);
-         SI != SE; ++SI) {
-      printEdgeProbability(OS << "  ", BI, *SI);
+    for (const BasicBlock *Succ : successors(BI)) {
+      printEdgeProbability(OS << "  ", BI, Succ);
----------------
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)

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:533
@@ -532,4 +532,3 @@
        BI != BE; ++BI) {
-    for (succ_const_iterator SI = succ_begin(BI), SE = succ_end(BI);
-         SI != SE; ++SI) {
-      printEdgeProbability(OS << "  ", BI, *SI);
+    for (const BasicBlock *Succ : successors(BI)) {
+      printEdgeProbability(OS << "  ", BI, Succ);
----------------
David Blaikie wrote:
> 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)
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.

http://reviews.llvm.org/D4481






More information about the llvm-commits mailing list