[PATCH] D152468: [NFC][DebugInfo][RemoveDIs] Use iterators over instruction pointers when using IRBuilder in various passes

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

LGTM (with a few nits)



================
Comment at: llvm/include/llvm/IR/BasicBlock.h:179
+  InstListType::const_iterator getFirstNonPHIIt() const;
+  InstListType::iterator getFirstNonPHIIt() {
+    BasicBlock::iterator It = static_cast<const BasicBlock *>(this)->getFirstNonPHIIt().getNonConst();
----------------
nit: please clang-format. Also, any reason that the `const` version is implemented in the .cpp file while this one is in the header?


================
Comment at: llvm/lib/IR/BasicBlock.cpp:224-229
+BasicBlock::const_iterator
+BasicBlock::getFirstNonPHIIt() const {
+  const Instruction *I = getFirstNonPHI();
+  BasicBlock::const_iterator It = I->getIterator();
+  return It;
+}
----------------



================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1836-1838
         if (BasicBlock *Preheader = L->getLoopPreheader())
-          InsertPt = Preheader->getTerminator();
-        else
+          InsertPt = Preheader->getTerminator()->getIterator();
+        else {
----------------
nit:

> Use braces for the `if` block to keep it uniform with the `else` block.

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2021
+            BasicBlock::iterator IP;
+            if (PHINode *PN = dyn_cast<PHINode>(OrigInc)) {
+              IP = PN->getParent()->getFirstInsertionPt();
----------------
nit: I don't think we need the new braces here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152468/new/

https://reviews.llvm.org/D152468