[clang-tidy] Clarify diagnostics of bugprone-sizeof-expression #95550
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...becasue they were strangely worded and in a few cases outright incorrect.
NagyDonat
requested review from
PiotrZSL,
whisperity,
EugeneZelenko and
Szelethus
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Donát Nagy (NagyDonat) Changes...becasue they were strangely worded and in a few cases outright incorrect. Patch is 33.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/95550.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index c25ee42d0899a..3afa0e5c39882 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -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(...)':"
+ " denominator differs from the size of array elements")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy == DenomTy) {
- // FIXME: This message is wrong, it should not refer to sizeof "pointer"
- // usage (and by the way, it would be to clarify all the messages).
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have the same type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (!WarnOnSizeOfPointer) {
// When 'WarnOnSizeOfPointer' is enabled, these messages become redundant:
if (PointedTy && DenomTy && PointedTy == DenomTy) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': size of pointer "
+ "is divided by size of pointed type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
DenomTy->isPointerType()) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have pointer types")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
}
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
index aef930f2c8fda..b898071a56613 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -34,24 +34,24 @@ int Test5() {
int sum = 0;
sum += sizeof(&S);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(__typeof(&S));
sum += sizeof(&TS);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(__typeof(&TS));
sum += sizeof(STRKWD MyStruct*);
sum += sizeof(__typeof(STRKWD MyStruct*));
sum += sizeof(TypedefStruct*);
sum += sizeof(__typeof(TypedefStruct*));
sum += sizeof(PTTS);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(PMyStruct);
sum += sizeof(PS);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(PS2);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(&A10);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
#ifdef __cplusplus
MyStruct &rS = S;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
index bfb2ec3a9eb02..37df21b6e0743 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
@@ -38,17 +38,17 @@ int Test1(const char* ptr) {
sum += sizeof(sum, LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: suspicious usage of 'sizeof(..., ...)'
sum += sizeof(AsBool());
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of integer type
sum += sizeof(AsInt());
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of integer type
sum += sizeof(AsEnum());
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of integer type
sum += sizeof(AsEnumClass());
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of integer type
sum += sizeof(M{}.AsInt());
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of integer type
sum += sizeof(M{}.AsEnum());
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of integer type
sum += sizeof(sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(LEN + sizeof(X));
@@ -62,31 +62,31 @@ int Test1(const char* ptr) {
sum += sizeof(LEN + - + -sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(char) / sizeof(char);
- // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)': both expressions have the same type
sum += sizeof(A) / sizeof(S);
- // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: suspicious usage of 'sizeof(...)/sizeof(...)': numerator is not a multiple of denominator
sum += sizeof(char) / sizeof(int);
- // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)': numerator is not a multiple of denominator
sum += sizeof(char) / sizeof(A);
- // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)': numerator is not a multiple of denominator
sum += sizeof(B[0]) / sizeof(A);
- // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)': numerator is not a multiple of denominator
sum += sizeof(ptr) / sizeof(char);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(ptr) / sizeof(ptr[0]);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(ptr) / sizeof(char*);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(ptr) / sizeof(void*);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(ptr) / sizeof(const void volatile*);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(ptr) / sizeof(char);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(int) * sizeof(char);
// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious 'sizeof' by 'sizeof' multiplication
sum += sizeof(ptr) * sizeof(ptr[0]);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: suspicious 'sizeof' by 'sizeof' multiplication
sum += sizeof(int) * (2 * sizeof(char));
// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious 'sizeof' by 'sizeof' multiplication
@@ -127,54 +127,54 @@ int Test5() {
int sum = 0;
sum += sizeof(&S.arr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(&kGlocalMyStruct.arr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(&kGlocalMyStructPtr->arr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(S.arr + 0);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(+ S.arr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof((int*)S.arr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(S.ptr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(kGlocalMyStruct.ptr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(kGlocalMyStructPtr->ptr);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(&kGlocalMyStruct);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(&S);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(MyStruct*);
sum += sizeof(PMyStruct);
sum += sizeof(PS);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(PS2);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(&A10);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(PtrArray) / sizeof(PtrArray[1]);
- // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(A10) / sizeof(PtrArray[0]);
sum += sizeof(PC) / sizeof(PtrArray[0]);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
- // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
+ // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: suspicious usage of 'sizeof(...)/sizeof(...)': both expressions have the same type
sum += sizeof(ArrayC) / sizeof(PtrArray[0]);
- // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious usage of 'sizeof(...)/sizeof(...)': numerator is not a multiple of denominator
sum += sizeof(PChar);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(PInt);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(PPInt);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(PPMyStruct);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
return sum;
}
@@ -200,9 +200,9 @@ void GenericFunctionTest() {
// reported by the `sizeof(pointer)` checks, but this causes some false
// positives, so it would be good to create an exception for them.
some_generic_function(&IntPP, sizeof(IntP));
- // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: suspicious usage of 'sizeof()' on an expression of pointer type
some_generic_function(&ClassPP, sizeof(ClassP));
- // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: suspicious usage of 'sizeof()' on an expression of pointer type
}
int ValidExpressions() {
@@ -222,7 +222,7 @@ int ValidExpressions() {
sum += sizeof(A[sizeof(A) / sizeof(int)]);
// Here the outer sizeof is reported, but the inner ones are accepted:
sum += sizeof(&A[sizeof(A) / sizeof(int)]);
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression of pointer type
sum += sizeof(sizeof(0)); // Special case: sizeof size_t.
sum += sizeof(void*);
sum += sizeof(void const *);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-warn-on-sizeof-pointer-to-aggregate.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-warn-on-sizeof-pointer-to-aggregate.cpp
index 5ef2c46ffbdf1..ed1dd06c8dc1d 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-warn-on-sizeof-pointer-to-aggregate.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-warn-on-sizeof-pointer-to-aggregate.cpp
@@ -68,9 +68,9 @@ int Test5() {
sum += sizeof(A10) / sizeof(PtrArray[0]);
// No warning.
sum += sizeof(PC) / sizeof(PtrArray[0]);
- // CHECK-MESSAGES: :[[@LIN...
[truncated]
|
|
This commit is motivated by the discussion on #94356 . Feel free to bikeshed the messages, I'm not too attached to the new choices, I'm just unsatisfied with the old ones. |
PiotrZSL
reviewed
Jun 14, 2024
SimplyDanny
approved these changes
Jun 15, 2024
I switched to ":" (instead of the ";" that was used previously) to emphasize that the second part of the message is an explanation/reason for the first part (and not a second, separate issue). However I can switch to "," or ";" if that option turns out to be more popular. @ rewiewers: Which symbol would you prefer in these messages? |
|
If there is no feedback on the separator mark bikeshedding (which is understandable -- it's a very minor question), I'll merge this commit on Thursday. |
The reason for the colon appears plausible to me. What I can't judge is if this is contradicting the style typically used in messages emitted by other checks. Be that as it may, it can be changed afterwards very easily if anyone is strict on that. So from my side, you can go ahead and merge this. |
|
Main reason why I pointed out that I do not like ':' character and would prefer ',' or ';' is that:
I would even prefer '-' character instead of ':' (in worst case) |
whisperity
reviewed
Jun 19, 2024
Comment on lines
351
to
353
| diag(E->getOperatorLoc(), | ||
| "suspicious usage of 'sizeof(array)/sizeof(...)':" | ||
| " denominator differs from the size of array elements") |
There was a problem hiding this comment.
Is the array case the only that can trigger this?
There was a problem hiding this comment.
Yes, this is only triggered when arrayType(hasElementType(recordType().bind("elem-type"))) matches, so we have an array type whose elements are records.
| diag(E->getOperatorLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)';" | ||
| " numerator is not a multiple of denominator") | ||
| diag(E->getOperatorLoc(), | ||
| "suspicious usage of 'sizeof(array)/sizeof(...)':" |
There was a problem hiding this comment.
Suggested change
| "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.
What do you think "division of two sizeof expression" instead of "sizeof(...)/sizeof(...)"
There was a problem hiding this comment.
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.
| @@ -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(...)':" | |||
There was a problem hiding this comment.
Suggested change
| diag(E->getOperatorLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)':" | |
| diag(E->getOperatorLoc(), "suspicious sizeof(...)/sizeof(...):" |
And then consistently everywhere.
There was a problem hiding this comment.
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.
|
Maybe the use of the Semicolons ( While a comma (
|
whisperity
added
the
enhancement
|
(@NagyDonat FYI you have a typo in your commit message, but I think because of the squashing policy it will not matter too much.) |
|
@PiotrZSL I see, thanks for the explanation, the "colons may confuse parsers" question is a good point. I think I'll return to using semicolons, because that's what was used before my commit. |
NagyDonat
requested review from
PiotrZSL,
SimplyDanny,
HerrCai0907 and
whisperity
PiotrZSL
approved these changes
Jul 1, 2024
|
I mentioned this change in the paragraph that was describing my earlier commit (that was also modifying this check). |
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…95550) … because they were strangely worded and in a few cases outright incorrect.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…95550) … because they were strangely worded and in a few cases outright incorrect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
… because they were strangely worded and in a few cases outright incorrect.