[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat closed https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: I mentioned this change in the paragraph that was describing my earlier commit (that was also modifying this check). https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/95550
From 06adc063c2388ea534537f5a417751fdf64b22cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Fri, 14 Jun 2024 15:16:34 +0200
Subject: [PATCH 1/4] [clang-tidy] Clarify diagnostics of
bugprone-sizeof-expression
...becasue they were strangely worded and in a few cases outright
incorrect.
---
.../bugprone/SizeofExpressionCheck.cpp| 22 ++---
.../checkers/bugprone/sizeof-expression-2.c | 12 +--
.../sizeof-expression-any-pointer.cpp | 86 +--
...on-warn-on-sizeof-pointer-to-aggregate.cpp | 4 +-
.../checkers/bugprone/sizeof-expression.cpp | 84 +-
5 files changed, 105 insertions(+), 103 deletions(-)
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 ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("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("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< 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(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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();
- //
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
=?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy Message-ID: In-Reply-To: https://github.com/PiotrZSL approved this pull request. Release notes entry would be welcome https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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(...)':"
NagyDonat wrote:
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.
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy
Message-ID:
In-Reply-To:
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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(...)':"
HerrCai0907 wrote:
What do you think "division of two sizeof expression" instead of
"sizeof(...)/sizeof(...)"
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/95550
From 06adc063c2388ea534537f5a417751fdf64b22cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Fri, 14 Jun 2024 15:16:34 +0200
Subject: [PATCH 1/3] [clang-tidy] Clarify diagnostics of
bugprone-sizeof-expression
...becasue they were strangely worded and in a few cases outright
incorrect.
---
.../bugprone/SizeofExpressionCheck.cpp| 22 ++---
.../checkers/bugprone/sizeof-expression-2.c | 12 +--
.../sizeof-expression-any-pointer.cpp | 86 +--
...on-warn-on-sizeof-pointer-to-aggregate.cpp | 4 +-
.../checkers/bugprone/sizeof-expression.cpp | 84 +-
5 files changed, 105 insertions(+), 103 deletions(-)
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 ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("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("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< 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(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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();
- //
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/95550
From 06adc063c2388ea534537f5a417751fdf64b22cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Fri, 14 Jun 2024 15:16:34 +0200
Subject: [PATCH 1/2] [clang-tidy] Clarify diagnostics of
bugprone-sizeof-expression
...becasue they were strangely worded and in a few cases outright
incorrect.
---
.../bugprone/SizeofExpressionCheck.cpp| 22 ++---
.../checkers/bugprone/sizeof-expression-2.c | 12 +--
.../sizeof-expression-any-pointer.cpp | 86 +--
...on-warn-on-sizeof-pointer-to-aggregate.cpp | 4 +-
.../checkers/bugprone/sizeof-expression.cpp | 84 +-
5 files changed, 105 insertions(+), 103 deletions(-)
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 ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("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("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< 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(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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();
- //
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: @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. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
NagyDonat wrote:
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.
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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")
NagyDonat wrote:
Yes, this is only triggered when
`arrayType(hasElementType(recordType().bind("elem-type")))` matches, so we have
an array type whose elements are records.
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/whisperity edited https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
whisperity wrote: (@NagyDonat FYI you have a typo in your commit message, but I think because of the squashing policy it will not matter too much.) https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/whisperity edited https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
whisperity wrote: Maybe the use of the `:` is indeed problematic, even if that would be the most accurate use of English in the present form, because of its use in the normal text-diagnostic-printing output. (Speaking from the English side of things, a `-` would also be incorrect, you would need a dash (`–`) but then that would not be nice on ASCII and whatnot...) https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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")
whisperity wrote:
Is the array case the only that can trigger this?
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
whisperity wrote:
```suggestion
diag(E->getOperatorLoc(), "suspicious sizeof(...)/sizeof(...):"
```
And then consistently everywhere.
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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(...)':"
whisperity wrote:
```suggestion
"suspicious usage of 'sizeof(...)/sizeof(...)':"
```
Maybe it would be better to keep the scheme here?
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
PiotrZSL wrote: Main reason why I pointed out that I do not like ':' character and would prefer ',' or ';' is that: - no other check is using it - could in theory cause an issue for some tools that parse warnings because ': ' characters are after "warning" and basically almost never happen in message itself. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
SimplyDanny wrote: > 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. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: 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. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: > Consider replacing ":" separator with "," 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? (My preferences are dot > keep the semicolon ≳ comma.) https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/SimplyDanny approved this pull request. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/HerrCai0907 approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/PiotrZSL commented: Consider replacing ":" separator with "," https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat created
https://github.com/llvm/llvm-project/pull/95550
...becasue they were strangely worded and in a few cases outright incorrect.
From 06adc063c2388ea534537f5a417751fdf64b22cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Fri, 14 Jun 2024 15:16:34 +0200
Subject: [PATCH] [clang-tidy] Clarify diagnostics of
bugprone-sizeof-expression
...becasue they were strangely worded and in a few cases outright
incorrect.
---
.../bugprone/SizeofExpressionCheck.cpp| 22 ++---
.../checkers/bugprone/sizeof-expression-2.c | 12 +--
.../sizeof-expression-any-pointer.cpp | 86 +--
...on-warn-on-sizeof-pointer-to-aggregate.cpp | 4 +-
.../checkers/bugprone/sizeof-expression.cpp | 84 +-
5 files changed, 105 insertions(+), 103 deletions(-)
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 ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("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("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< 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(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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
@@
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: This commit is motivated by the discussion on https://github.com/llvm/llvm-project/pull/94356 . Feel free to bikeshed the messages, I'm not too attached to the new choices, I'm just unsatisfied with the old ones. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
llvmbot wrote:
@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:
- (modified) clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
(+12-10)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
(+6-6)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
(+43-43)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-warn-on-sizeof-pointer-to-aggregate.cpp
(+2-2)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
(+42-42)
``diff
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 ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("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("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< 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(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
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
+++
