[clang-tidy] fix bugprone-sizeof-expression when sizeof expression with template types #115275
Conversation
…th template types Fixed: llvm#115175. `dependent type` are not the same even pointers are the same.
|
@llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) ChangesFixed: #115175. Full diff: https://github.com/llvm/llvm-project/pull/115275.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 628d30ce7f73fe..7269683384f6b5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -400,7 +400,9 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
"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) {
+ } else if (NumTy && DenomTy && NumTy == DenomTy &&
+ !NumTy->isDependentType()) {
+ // dependent type should not be compared.
diag(E->getOperatorLoc(),
"suspicious usage of 'sizeof(...)/sizeof(...)'; both expressions "
"have the same type")
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index abcdcc25705bf5..4fa8292ff89745 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -175,7 +175,8 @@ Changes in existing checks
- Improved :doc:`bugprone-sizeof-expression
<clang-tidy/checks/bugprone/sizeof-expression>` check to find suspicious
usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or
- subtracting from a pointer directly or when used to scale a numeric value.
+ subtracting from a pointer directly or when used to scale a numeric value and
+ fix false positive when sizeof expression with template types.
- Improved :doc:`bugprone-unchecked-optional-access
<clang-tidy/checks/bugprone/unchecked-optional-access>` to support
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
index 81efd87345c97e..f3ab78c7355b28 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -385,3 +385,8 @@ int ValidExpressions() {
sum += sizeof(PtrArray) / sizeof(A[0]);
return sum;
}
+
+template<class T>
+int ValidateTemplateTypeExpressions(T t) {
+ return sizeof(t.val) / sizeof(t.val[0]);
+}
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) ChangesFixed: #115175. Full diff: https://github.com/llvm/llvm-project/pull/115275.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 628d30ce7f73fe..7269683384f6b5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -400,7 +400,9 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
"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) {
+ } else if (NumTy && DenomTy && NumTy == DenomTy &&
+ !NumTy->isDependentType()) {
+ // dependent type should not be compared.
diag(E->getOperatorLoc(),
"suspicious usage of 'sizeof(...)/sizeof(...)'; both expressions "
"have the same type")
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index abcdcc25705bf5..4fa8292ff89745 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -175,7 +175,8 @@ Changes in existing checks
- Improved :doc:`bugprone-sizeof-expression
<clang-tidy/checks/bugprone/sizeof-expression>` check to find suspicious
usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or
- subtracting from a pointer directly or when used to scale a numeric value.
+ subtracting from a pointer directly or when used to scale a numeric value and
+ fix false positive when sizeof expression with template types.
- Improved :doc:`bugprone-unchecked-optional-access
<clang-tidy/checks/bugprone/unchecked-optional-access>` to support
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
index 81efd87345c97e..f3ab78c7355b28 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -385,3 +385,8 @@ int ValidExpressions() {
sum += sizeof(PtrArray) / sizeof(A[0]);
return sum;
}
+
+template<class T>
+int ValidateTemplateTypeExpressions(T t) {
+ return sizeof(t.val) / sizeof(t.val[0]);
+}
|
| subtracting from a pointer directly or when used to scale a numeric value and | ||
| fix false positive when sizeof expression with template types. |
There was a problem hiding this comment.
If there were no releases since this change, then there is no need for the release note entry. Basically you're hotfixing a change that is coming out in the same hypothetical next release.
If the change in this patch is not related to what is mentioned in this changelog entry, then it is worth a full new changelog entry, like this:
| subtracting from a pointer directly or when used to scale a numeric value and | |
| fix false positive when sizeof expression with template types. | |
| subtracting from a pointer directly or when used to scale a numeric value. | |
| - Fixed :doc:`bugprone-sizeof-expression | |
| <clang-tidy/checks/bugprone/sizeof-expression>` check not to report on | |
| ``sizeof()/sizeof()`` expressions for template types. |
There was a problem hiding this comment.
I think not. clang-tidy release-note always combines the all changes of rule in one item instead of spilt them.
There was a problem hiding this comment.
example:
readability-container-contains
readability-enum-initial-value
There was a problem hiding this comment.
If there were no releases since this change, then there is no need for the release note entry. Basically you're hotfixing a change that is coming out in the same hypothetical next release.
I'm not sure what you meant here. There should be a release note, unless the issue was introduced in the current, release cycle. The diagnostic reproduces in clang-tidy 18 for me, or did you mean something different (e.g., general in-case-of comment)?
clang-tidy release-note always combines the all changes of rule in one item instead of spilt them.
+1, that's the current approach
There was a problem hiding this comment.
If there were no releases since this change, then there is no need for the release note entry. Basically you're hotfixing a change that is coming out in the same hypothetical next release.
I'm not sure what you meant here. There should be a release note, unless the issue was introduced in the current, release cycle. The diagnostic reproduces in clang-tidy 18 for me, or did you mean something different (e.g., general in-case-of comment)?
Because of the merged release note I thought this is related to #106061, hence the confusion.
clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: whisperity <[email protected]>
| template<class T> | ||
| int ValidateTemplateTypeExpressions(T t) { | ||
| return sizeof(t.val) / sizeof(t.val[0]); | ||
| } |
There was a problem hiding this comment.
Please note the issue somewhere on this test-case (115175) (e.g. namespace)
HerrCai0907
deleted the
115175-clang-tidy-bugprone-sizeof-expression-false-positives
branch
…th template types (llvm#115275) Fixed: llvm#115175. `dependent type` are not the same even pointers are the same. --------- Co-authored-by: whisperity <[email protected]>
Fixed: #115175.
dependent typeare not the same even pointers are the same.