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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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())
----------------
mtrofin wrote:
> 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'.
Hmm - I don't see it. There's an `I` at 560, but that scope ends at 590 - if `I` is good enough for that scope, it's good enough for this one?

Also I think it's not uncommon to use `J`, `K` etc for single name index-y variables after `I`, but more common if the loops are tightly coupled together - would be a bit weird to use them when they're further apart.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:630
 
-      int InlineHistoryID;
+      int InlineHistoryID = -1;
       if (!IsTriviallyDead) {
----------------
davidxl wrote:
> mtrofin wrote:
> > 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.
> > 
> > 
> This is a good point.  It is ok to initialize it to  illegal value that can be caught by assertions or can consistently crash the program. Here is not the case: -1 is actually a valid value to indicate a state, so it is not a good idea to initialize it to it.
Yeah, mixed opinions.

Really pedantically you could use Optional<int> which will ensure it's checked in +Asserts build, or I guess some other invalid value (like -2), though not sure how much that'd help - it'd suppress msan and make such bugs much more elusive - maybe an infinite loop later on if inlineHistoryIncludes was called with -2? Oh, maybe buffer underrun which asan would catch.


================
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) {
----------------
mtrofin wrote:
> 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 :)
I see an `I` from 947 to 963, but not one that seems to overlap with this line/scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77837