-
Notifications
You must be signed in to change notification settings - Fork 15.9k
[clang-tidy] Clarify diagnostics of bugprone-sizeof-expression #95550
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -296,7 +296,7 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) { | |||||
| } else if (const auto *E = | ||||||
| Result.Nodes.getNodeAs<Expr>("sizeof-integer-call")) { | ||||||
| diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression " | ||||||
| "that results in an integer") | ||||||
| "of integer type") | ||||||
| << E->getSourceRange(); | ||||||
| } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-this")) { | ||||||
| diag(E->getBeginLoc(), | ||||||
|
|
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) { | |||||
| << E->getSourceRange(); | ||||||
| } else { | ||||||
| diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression " | ||||||
| "that results in a pointer") | ||||||
| "of pointer type") | ||||||
| << E->getSourceRange(); | ||||||
| } | ||||||
| } else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>( | ||||||
|
|
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) { | |||||
|
|
||||||
| if (DenominatorSize > CharUnits::Zero() && | ||||||
| !NumeratorSize.isMultipleOf(DenominatorSize)) { | ||||||
| diag(E->getOperatorLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)';" | ||||||
| diag(E->getOperatorLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)':" | ||||||
| " numerator is not a multiple of denominator") | ||||||
| << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); | ||||||
| } else if (ElementSize > CharUnits::Zero() && | ||||||
| DenominatorSize > CharUnits::Zero() && | ||||||
| ElementSize != DenominatorSize) { | ||||||
| diag(E->getOperatorLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)';" | ||||||
| " numerator is not a multiple of denominator") | ||||||
| diag(E->getOperatorLoc(), | ||||||
| "suspicious usage of 'sizeof(array)/sizeof(...)':" | ||||||
|
||||||
| "suspicious usage of 'sizeof(array)/sizeof(...)':" | |
| "suspicious usage of 'sizeof(...)/sizeof(...)':" |
Maybe it would be better to keep the scheme here?
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.
What do you think "division of two sizeof expression" instead of "sizeof(...)/sizeof(...)"
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.
It would be slightly better in isolation, but all the other diagnostics look like e.g. suspicious usage of 'sizeof(..., ...)' or suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)' so for the sake of consistency I'd like to leave this aspect of the messages unchanged.
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.
Is the array case the only that can trigger this?
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.
Yes, this is only triggered when arrayType(hasElementType(recordType().bind("elem-type"))) matches, so we have an array type whose elements are records.
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.
And then consistently everywhere.
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 kept "usage of" and the apostrophes to be consistent with the rest of the messages emitted by this checker. If you think that this is important, it would be possible to extend the scope of this commit to apply these changes in all the other messages.