[PATCH] D77837: [llvm][NFC] Style fixes in Inliner.cpp
Mircea Trofin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 9 17:26:21 PDT 2020
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
More information about the llvm-commits
mailing list