Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] Create a check for signed and unsigned integers comparison #113144
base: main
Are you sure you want to change the base?
[clang-tidy] Create a check for signed and unsigned integers comparison #113144
Changes from 4 commits
34ee6f5
b013b2c
d9a6540
c4cbfb7
47cffa5
0f320e4
be56a62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If instead, you provide the header as an option and a namespace, then the check would be even more generic and the Qt module would not be needed (not saying a Qt module wouldn't make sense, just that this check may not require adding it). Though a quick look at boost, gsl and abseil did not show them having such functions.
(You could mention the settings for QT in the documentation as an example)
It would be best to check what others think, though. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be comfortable to have QT module, because in that case it's possible to turn on several QT-related checks together at the same moment (without providing parameters for each check from command line).
Yes, there is only 1 check that has QT-related extention for now, but I have several check-ideas....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add
unless(isInstantiationDependent())
intobinaryOperator
. We would not want to replace the code of a template that is instantiated with integral types, and types that have an overloaded operator implemented.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I am not sure if I did correctly, but I use: isInTemplateInstantiation(). Is it enough? Or is it still better to add isInstantiationDependent()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you determine LHS and RHS like this, the replacement will include the explicit cast if there is any, while it could have removed that cast instead. With the below comment on the fixit, you'd simply need to know which of
sIntCastExpression
anduIntCastExpression
(also voiding my comment on it being unused) is the LHS and which is the RHS, allowing you, e.g., to replace instead of insert thestd::cmp_*(
instead of inserting it at the frontThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure If I did everything 100% according the comment, but please see the change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating a large fix, it is preferred to try and be more surgical, which can be done in this case.
That is, insert
(CmpNamespace+parseOpCode(OpCode)+"(").str()
beforeLHS
, replaceBinaryOp->getOperatorLoc()
with a,
, and add a)
after RHS. That way, there is no need to query the Lexer (performance) and fixes from other checks that operate inside e.g.,LHS
, may be applied as well that would otherwise be blocked due to overlapping fixit ranges.Also note that string concatenation is done via an
llvm::Twine
:(CmpNamespace+parseOpCode(OpCode)+"(").str()
, which can cobine c-strings,llvm::StringRef
,std::string
more efficiently (but a good rule of thumb is to always call.str()
and have the Twine always be a temporary)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split the lage fix. Btw, I noticed a little bit strange behavior in tests:
check-clang-tidy.py uses "-strict-whitespace" option.
It seems because of that opthion, CreateReplacement(BinaryOp->getOperatorLoc(), ", ") doesn't replace whitespaces of operators (" == ", " <= ", etc).
If run without "-strict-whitespace" option, then CreateReplacement() replaces with whitespaces.
I didn't find how to fix it, so you can notice there are more whitespaces in tests, then it had before.