[llvm-commits] [PATCH] [msan] Remove unreachable bb's

[Chandler Carruth]

  Sorry, I saw two things that I looked for, and didn't realize those were the  *only* two things left. =[ I shouldn't have jumped to a conclusion.

  Anyways, I've pinged those comments below.


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:730-732
@@ +729,5 @@
+
+/// RemoveUnreachableBlocks - Remove all blocks that can not be reached from the
+/// function's entry.
+void llvm::RemoveUnreachableBlocks(Function &F) {
+  SmallPtrSet<BasicBlock*, 128> Reachable;
----------------
Chandler Carruth wrote:
> This makes more sense in Local.h and Local.cpp than BasicBlockUtils.cpp -- this isn't for transforming a basic block, but a function.
> 
> Also, it should start with lower case letter as 'removeUnreachableBlocks', and it's comment should be in the modern doxygen style the coding standards recommend.
Note the comment about doxygen style according to the coding standards.

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:748
@@ +747,3 @@
+  assert(Reachable.size() < F.size());
+  for (Function::iterator BB = ++F.begin(); BB != F.end();)
+    if (!Reachable.count(BB)) {
----------------
Chandler Carruth wrote:
> ++ is confusing (and may be risky) here -- what guarantee do you have that begin() returns a temporary which can be mutated?
> 
> I would use llvm::next(F.begin()), this is also the more idiomatic way to do this.
> 
> Also, this should be written using the idiomatic LLVM loop pattern:
> 
>   for (Function::iterator I = llvm::next(F.begin()), E = F.end(); I != E;)
> 
> The end iterator is stable even across basic blocks being removed.
I still think using I or BI or BBI, or anything with the I in it to indicate that this is an iterator would be good. I would personally use 'I' and rename the variable below from 'I' to something else.


http://llvm-reviews.chandlerc.com/D208