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

[David Blaikie via Phabricator via llvm-commits]

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

Looks good - few optional bits & pieces (the initialization's probably something I feel pretty strongly about - but naming you can take/leave as you see fit).

In general fixing names to match the LLVM naming conventions - feel free to commit that sort of thing without review, usually independent changes independently - but if you're fixing a class of bugs (like a whole bunch of functions in one class, etc) you can do it in pretty big chunks (whole class/file at a time, potentially - but don't feel like you have to fix everything or nothing).



================
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())
----------------
"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


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:630
 
-      int InlineHistoryID;
+      int InlineHistoryID = -1;
       if (!IsTriviallyDead) {
----------------
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)


================
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) {
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77837