[clang] [llvm] [mlir] Fix mismatches between function parameter definitions and declarations (PR #89512)

Troy Butler via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 24 15:57:21 PDT 2024


Troy-Butler wrote:

> > 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](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) 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. 

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


More information about the llvm-commits mailing list