Skip to content

[clang-tidy] fix bugprone-sizeof-expression when sizeof expression with template types #115275

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.

Sign up for GitHub

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

Merged
HerrCai0907 merged 3 commits into from
Nov 12, 2024
Merged

[clang-tidy] fix bugprone-sizeof-expression when sizeof expression with template types #115275

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06fb72b
[clang-tidy] fix bugprone-sizeof-expression when sizeof expression wi…
HerrCai0907 Nov 7, 2024
2df4bc1
Update clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
HerrCai0907 Nov 9, 2024
4388d53
add issue number in test
HerrCai0907 Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
3 changes: 2 additions & 1 deletion clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +178 to +179
Copy link
Member

@whisperity whisperity Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

Copy link
Contributor Author

@HerrCai0907 HerrCai0907 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not. clang-tidy release-note always combines the all changes of rule in one item instead of spilt them.

Copy link
Contributor Author

@HerrCai0907 HerrCai0907 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example:
readability-container-contains
readability-enum-initial-value

Copy link
Contributor

@5chmidti 5chmidti Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@whisperity whisperity Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- Improved :doc:`bugprone-unchecked-optional-access
<clang-tidy/checks/bugprone/unchecked-optional-access>` to support
Expand Down
5 changes: 5 additions & 0 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
Comment on lines +390 to +393
Copy link
Contributor

@5chmidti 5chmidti Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note the issue somewhere on this test-case (115175) (e.g. namespace)

Loading