llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3804: lacking () for clarity #85868
Comments
|
@llvm/issue-subscribers-backend-x86 Author: None (dcb314)
Static analyser cppcheck says:
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3804:47: style: Clarify calculation precedence for '&' and '?'. [clarifyCalculation] Source code is I am guessing that was intended. Suggest add () for clarity of intention. |
RKSimon
added
the
good first issue
|
Hi! This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:
If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below. |
|
@llvm/issue-subscribers-good-first-issue Author: None (dcb314)
Static analyser cppcheck says:
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3804:47: style: Clarify calculation precedence for '&' and '?'. [clarifyCalculation] Source code is I am guessing that was intended. Suggest add () for clarity of intention. |
|
It may be useful to wait for #84481 and extend it bitwise operators, so such code patterns could be improved on entire code base. |
|
@EugeneZelenko Do we have any working clang-tidy report of the codebase? https://llvm.org/reports/scan-build/ has barely worked for years now........ |
IMHO, doing this for arithmetic operators isn't as useful as doing it for the other operators. Most people remember basic school material, but an average C programmer has only I find the bit twiddling and ? : operators particularly troublesome. K&R does mention I counted 62 examples across the llvm code base where cppcheck didn't like the look |
|
Hi, with a quick look at the precedence table for C++, the proposal looks accurate. On a second phase I can create a pull request with the warnings from CppCheck on ternary operators. |
Here is the current list from cppcheck: trunk/clang/lib/Basic/Targets/AMDGPU.cpp:235:62: style: Clarify calculation precedence for '&' and '?'. [clarifyCalculation] Some, all or none of these might be worth improving for clarity. |
|
Hi @dcb314 , @EugeneZelenko can you assign me this issue? |
|
Is the plan on this ticket to fix all the reports from cppcheck or update clang-tidy to report them in a similar way? |
|
Suggest a) fix this one style issue For b) I would be particularly careful to only make changes that do not change I can't be sure that, if the meaning of the code were accidentally changed by |
I know nothing about clang-tidy, so I should not comment on it. However, I would do a) and b) first, which are existing problems. |
…xpressions. (#90391) Fixes [#85868](#85868) Parenthesis are added as requested on ternary operators with non trivial conditions. I used this [precedence table](https://en.cppreference.com/w/cpp/language/operator_precedence) for reference, to make sure we get the expected behavior on each change.
|
The ternary conditional pattern Adding parentheses around these expressions seems to provide little benefit in terms of readability or maintainability, given the churn cost. Consider suppressing these static analyzer reports for this specific pattern. |
Disagree. Does the specified code mean (I == E - 1) ? Size : Layout->getElementOffset(I + 1) or I == ((E - 1) ? Size : Layout->getElementOffset(I + 1)) ? Whatever the right answer is, are you sure a sufficient proportion of coders Making code obvious, without too many (), matters. |
…xpressions. (llvm#90391) Fixes [llvm#85868](llvm#85868) Parenthesis are added as requested on ternary operators with non trivial conditions. I used this [precedence table](https://en.cppreference.com/w/cpp/language/operator_precedence) for reference, to make sure we get the expected behavior on each change.
Many coders are familiar with the relative precedence of ( I believe the majority of this code base doesn't add parens for such code patterns and there are potentially many users disagreeing with this change. |
Static analyser cppcheck says:
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3804:47: style: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
Source code is
I am guessing that
was intended. Suggest add () for clarity of intention.
Not all of us know all the C precedence rules well.
The text was updated successfully, but these errors were encountered: