[clang] [llvm] [mlir] Fix mismatches between function parameter definitions and declarations (PR #89512)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 24 14:53:46 PDT 2024
NagyDonat 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.
https://github.com/llvm/llvm-project/pull/89512
More information about the cfe-commits
mailing list