Fix mismatches between function parameter definitions and declarations

[Troy-Butler]

Addresses issue #88716.

Some function parameter names in the affected header files did not match the parameter names in the definitions, or were listed in a different order.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Troy Butler (Troy-Butler)

Changes

Addresses issue #88716.

---

Full diff: https://github.com/llvm/llvm-project/pull/89512.diff

4 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h (+2-2)
• (modified) llvm/lib/Transforms/InstCombine/InstCombineInternal.h (+1-1)
• (modified) llvm/lib/Transforms/Vectorize/VPlan.h (+2-2)
• (modified) mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h (+3-3)

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
index fac0c04ae2caab..e60a49f68b7a0d 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
@@ -227,12 +227,12 @@ class StoreManager {
   ///   information will not be used.
   virtual StoreRef invalidateRegions(Store store,
                                   ArrayRef<SVal> Values,
-                                  const Expr *E, unsigned Count,
+                                  const Expr *Ex, unsigned Count,
                                   const LocationContext *LCtx,
                                   const CallEvent *Call,
                                   InvalidatedSymbols &IS,
                                   RegionAndSymbolInvalidationTraits &ITraits,
-                                  InvalidatedRegions *InvalidatedTopLevel,
+                                  InvalidatedRegions *TopLevelRegions,
                                   InvalidatedRegions *Invalidated) = 0;
 
   /// enterStackFrame - Let the StoreManager to do something when execution
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
index 4479afbd09afde..4ec5f417998273 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
+++ b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
@@ -433,7 +433,7 @@ class LLVM_LIBRARY_VISIBILITY InstCombinerImpl final
   Value *foldAndOrOfICmpsOfAndWithPow2(ICmpInst *LHS, ICmpInst *RHS,
                                        Instruction *CxtI, bool IsAnd,
                                        bool IsLogical = false);
-  Value *matchSelectFromAndOr(Value *A, Value *B, Value *C, Value *D,
+  Value *matchSelectFromAndOr(Value *A, Value *C, Value *B, Value *D,
                               bool InvertFalseVal = false);
   Value *getSelectCondition(Value *A, Value *B, bool ABIsTheSame);
 
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index c74329a0bcc4ac..21b088cd238660 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -3278,8 +3278,8 @@ class VPlan {
 private:
   /// Add to the given dominator tree the header block and every new basic block
   /// that was created between it and the latch block, inclusive.
-  static void updateDominatorTree(DominatorTree *DT, BasicBlock *LoopLatchBB,
-                                  BasicBlock *LoopPreHeaderBB,
+  static void updateDominatorTree(DominatorTree *DT, BasicBlock *LoopHeaderBB,
+                                  BasicBlock *LoopLatchBB,
                                   BasicBlock *LoopExitBB);
 };
 
diff --git a/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h b/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h
index 9d69a233555986..38f8c8423fd16f 100644
--- a/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h
+++ b/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h
@@ -284,9 +284,9 @@ class SparseIterator {
 };
 
 /// Helper function to create a TensorLevel object from given `tensor`.
-std::unique_ptr<SparseTensorLevel> makeSparseTensorLevel(OpBuilder &builder,
-                                                         Location loc, Value t,
-                                                         unsigned tid, Level l);
+std::unique_ptr<SparseTensorLevel> makeSparseTensorLevel(OpBuilder &b,
+                                                         Location l, Value t,
+                                                         unsigned tid, Level lvl);
 
 /// Helper function to create a simple SparseIterator object that iterate over
 /// the SparseTensorLevel.

[llvmbot]

@llvm/pr-subscribers-mlir-sparse

Author: Troy Butler (Troy-Butler)

Changes

Addresses issue #88716.

---

Full diff: https://github.com/llvm/llvm-project/pull/89512.diff

4 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h (+2-2)
• (modified) llvm/lib/Transforms/InstCombine/InstCombineInternal.h (+1-1)
• (modified) llvm/lib/Transforms/Vectorize/VPlan.h (+2-2)
• (modified) mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h (+3-3)

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
index fac0c04ae2caab..e60a49f68b7a0d 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
@@ -227,12 +227,12 @@ class StoreManager {
   ///   information will not be used.
   virtual StoreRef invalidateRegions(Store store,
                                   ArrayRef<SVal> Values,
-                                  const Expr *E, unsigned Count,
+                                  const Expr *Ex, unsigned Count,
                                   const LocationContext *LCtx,
                                   const CallEvent *Call,
                                   InvalidatedSymbols &IS,
                                   RegionAndSymbolInvalidationTraits &ITraits,
-                                  InvalidatedRegions *InvalidatedTopLevel,
+                                  InvalidatedRegions *TopLevelRegions,
                                   InvalidatedRegions *Invalidated) = 0;
 
   /// enterStackFrame - Let the StoreManager to do something when execution
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
index 4479afbd09afde..4ec5f417998273 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
+++ b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
@@ -433,7 +433,7 @@ class LLVM_LIBRARY_VISIBILITY InstCombinerImpl final
   Value *foldAndOrOfICmpsOfAndWithPow2(ICmpInst *LHS, ICmpInst *RHS,
                                        Instruction *CxtI, bool IsAnd,
                                        bool IsLogical = false);
-  Value *matchSelectFromAndOr(Value *A, Value *B, Value *C, Value *D,
+  Value *matchSelectFromAndOr(Value *A, Value *C, Value *B, Value *D,
                               bool InvertFalseVal = false);
   Value *getSelectCondition(Value *A, Value *B, bool ABIsTheSame);
 
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index c74329a0bcc4ac..21b088cd238660 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -3278,8 +3278,8 @@ class VPlan {
 private:
   /// Add to the given dominator tree the header block and every new basic block
   /// that was created between it and the latch block, inclusive.
-  static void updateDominatorTree(DominatorTree *DT, BasicBlock *LoopLatchBB,
-                                  BasicBlock *LoopPreHeaderBB,
+  static void updateDominatorTree(DominatorTree *DT, BasicBlock *LoopHeaderBB,
+                                  BasicBlock *LoopLatchBB,
                                   BasicBlock *LoopExitBB);
 };
 
diff --git a/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h b/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h
index 9d69a233555986..38f8c8423fd16f 100644
--- a/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h
+++ b/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorIterator.h
@@ -284,9 +284,9 @@ class SparseIterator {
 };
 
 /// Helper function to create a TensorLevel object from given `tensor`.
-std::unique_ptr<SparseTensorLevel> makeSparseTensorLevel(OpBuilder &builder,
-                                                         Location loc, Value t,
-                                                         unsigned tid, Level l);
+std::unique_ptr<SparseTensorLevel> makeSparseTensorLevel(OpBuilder &b,
+                                                         Location l, Value t,
+                                                         unsigned tid, Level lvl);
 
 /// Helper function to create a simple SparseIterator object that iterate over
 /// the SparseTensorLevel.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Troy-Butler]

I do not have the ability to merge, so if everything looks good with my pull request, can the last reviewer please merge this? Thank you!

[NagyDonat]

The pre-commit version of the function declaration listed the parameters in the order A, C, B, D to indicate the expected grouping of arguments, i.e. (A & C) | (B & D). Changing the order to A, B, C, D would have required reworking the function, which seemed outside the scope of this issue - so I opted to reflect the A, C, B, D order in the function declaration instead. However, as requested, I have modified the parameter names, listed them in logically ascending order, and reworked the function to match.

Thanks for explaining the situation and updating the code!

I think declaring the variable names in an alphabetical order will make the code less surprising and easier to read. However, now that I know more about the context where these variables are used, I think returning to the single-letter A, B, C and D would be better than the valA, valB, valC, valD that you introduced for two reasons:

• Previously I neglected to look at the definition of this function; now that I see that it's dealing with abstract Boolean predicates, I think that it's a good choice to use the single-letter names that are customary as mathematical notation.
• There is also a design rule which says that variables need to be capitalized, so the lowercase val prefix should not be introduced. (BTW this rule is not absolute: in existing code that uses a different naming convention, it's permissible/preferred to use that convention for the sake of consistency.)

I know that you probably introduced the val prefix because of my complaint about the "meaningless single letter" variable names; please excuse me for these back-and-forth suggestions.

[Troy-Butler]

The pre-commit version of the function declaration listed the parameters in the order A, C, B, D to indicate the expected grouping of arguments, i.e. (A & C) | (B & D). Changing the order to A, B, C, D would have required reworking the function, which seemed outside the scope of this issue - so I opted to reflect the A, C, B, D order in the function declaration instead. However, as requested, I have modified the parameter names, listed them in logically ascending order, and reworked the function to match.

Thanks for explaining the situation and updating the code!

I think declaring the variable names in an alphabetical order will make the code less surprising and easier to read. However, now that I know more about the context where these variables are used, I think returning to the single-letter A, B, C and D would be better than the valA, valB, valC, valD that you introduced for two reasons:

• Previously I neglected to look at the definition of this function; now that I see that it's dealing with abstract Boolean predicates, I think that it's a good choice to use the single-letter names that are customary as mathematical notation.
• There is also a design rule which says that variables need to be capitalized, so the lowercase val prefix should not be introduced. (BTW this rule is not absolute: in existing code that uses a different naming convention, it's permissible/preferred to use that convention for the sake of consistency.)

I know that you probably introduced the val prefix because of my complaint about the "meaningless single letter" variable names; please excuse me for these back-and-forth suggestions.

No worries, I understand. I've reverted the parameter variable names back to single letter form.

[NagyDonat]

Thanks for the update! I'll merge the PR within ~a day if nobody objects.

[NagyDonat]

@Troy-Butler
I started to do the merge, but I cancelled it for now because it seems that github mangles the email metadata:

(Note that I obscured your email address in this screenshot, but it can be found within the commits that you published.)

• I'd guess that want to use your "real" email address as the Author of this commit -- that case you probably need to change your github account settings. (I don't know the exact details but IIRC you can toggle whether it will use your email address in your commits.) Ping me when you're ready and I'll check the merge again.
• If you don't want to embed that email address into the LLVM git history, then I can merge the commit without the Signed-off-by and Co-authored-by lines that mention it. The address will remain on the internet (in this pull request), but it won't be part of the LLVM main branch.

[Troy-Butler]

@Troy-Butler I started to do the merge, but I cancelled it for now because it seems that github mangles the email metadata: (Note that I obscured your email address in this screenshot, but it can be found within the commits that you published.)

• I'd guess that want to use your "real" email address as the Author of this commit -- that case you probably need to change your github account settings. (I don't know the exact details but IIRC you can toggle whether it will use your email address in your commits.) Ping me when you're ready and I'll check the merge again.
• If you don't want to embed that email address into the LLVM git history, then I can merge the commit without the Signed-off-by and Co-authored-by lines that mention it. The address will remain on the internet (in this pull request), but it won't be part of the LLVM main branch.

Thank you for pointing that out. I don't have any issues with that email address being embedded in LLVM's git history - you can go ahead and commit. Thanks!