[PATCH] D77837: [llvm][NFC] Style fixes in Inliner.cpp

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 4 inline comments as done.
mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:598
   unsigned FirstCallInSCC = CallSites.size();
-  for (unsigned i = 0; i < FirstCallInSCC; ++i)
-    if (Function *F = CallSites[i].first->getCalledFunction())
+  for (unsigned Index = 0; Index < FirstCallInSCC; ++Index)
+    if (Function *F = CallSites[Index].first->getCalledFunction())
----------------
dblaikie wrote:
> "I" would suffice here (in general I think of shorter variables for shorter scopes) - the LLVM style guide does provide examples with single uppercase (I thought there used to be some special case for lower case index variables - maybe that was removed with the advent of range-based for loops and such, or I'm misremembering): https://llvm.org/docs/CodingStandards.html#turn-predicate-loops-into-predicate-functions
There's a name collision for 'I'.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:630
 
-      int InlineHistoryID;
+      int InlineHistoryID = -1;
       if (!IsTriviallyDead) {
----------------
dblaikie wrote:
> Generally I'm in favor of not adding unneeded (in the sense that if the program is working as intended it won't read uninitialized values) - that way tools like MSan and the like can find bugs in this code if the program isn't working as intended.
> 
> Looks like that's the case here - InlineHistoryID is only used under the same condition (IsTriviallyDead) it's initialized. (also Clang has warnings for this too, in addition to the runtime checks)
My worry is maintainability - initializing is local, while making sure that code evolution doesn't lead to uninitialized value propagating isn't.




================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:986
   // inlining can introduce new calls that need to be processed.
-  for (int i = 0; i < (int)Calls.size(); ++i) {
+  for (int CallsiteIndex = 0; CallsiteIndex < (int)Calls.size();
+       ++CallsiteIndex) {
----------------
dblaikie wrote:
> Again, probably using just `I` is fine here. On the other hand it is a rather long loop that does some non-trivial things with this loop index, so if you prefer the longer name - I quite understand. 
Also name collision, unfortunately - I also didn't like I had to cook up these long names :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77837