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