[PATCH] D141568: [SCEV] Support SMin/Umin for GetMinTrailingZeros

[Joshua Cao via Phabricator via llvm-commits]

caojoshua added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6346
     uint32_t MinOpRes = GetMinTrailingZeros(M->getOperand(0));
-    for (unsigned i = 1, e = M->getNumOperands(); MinOpRes && i != e; ++i)
-      MinOpRes = std::min(MinOpRes, GetMinTrailingZeros(M->getOperand(i)));
+    for (unsigned I = 1; MinOpRes && I != M->getNumOperands(); ++I)
+      MinOpRes = std::min(MinOpRes, GetMinTrailingZeros(M->getOperand(I)));
----------------
caojoshua wrote:
> nikic wrote:
> > Any reason to deviate from the previous structure?
> `i` -> `I` because [LLVM coding standards state](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) that `Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)`. And cause my editor is smart enough to know we are using LLVM coding standards and recommend this change.
> 
> No need to assign assign `getNumOperands()`  to `E`. Can just compare directly against `getNumOperands()`, and remove unnecessary `E` variable. M is `const`, so we know the number of operands will not change through iterations barring some unsafe casting (I think this is how C++ const works).
> 
> No strong opinions from me. This deviates from code style in rest of function. I can update the rest of the function as a NFC followup to make sure everything is consistent.
I reverted the previous structure, but kept the variable names capitalized


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141568