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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
