Skip to content

[clang-tidy][misc-const-correctness] fix fp when using const array type. #133018

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
merged 5 commits into from
Mar 27, 2025
Merged

[clang-tidy][misc-const-correctness] fix fp when using const array type. #133018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2defc6
[clang-tidy][misc-const-correctness] fix fp when using const array type.
HerrCai0907 Mar 25, 2025
166e457
Update clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
HerrCai0907 Mar 26, 2025
42466ed
Update clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
HerrCai0907 Mar 26, 2025
8352c50
fix with review
HerrCai0907 Mar 27, 2025
417b29d
Merge branch '132931-clang-tidy-suggests-adding-const-to-arrays-that-…
HerrCai0907 Mar 27, 2025
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
8 changes: 4 additions & 4 deletions clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,10 @@ void ConstCorrectnessCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
}

void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
const auto ConstType = hasType(
qualType(isConstQualified(),
// pointee check will check the const pointer and const array
unless(pointerType()), unless(arrayType())));
const auto ConstType =
hasType(qualType(isConstQualified(),
// pointee check will check the constness of pointer
unless(pointerType())));

const auto ConstReference = hasType(references(isConstQualified()));
Copy link
Contributor

Choose a reason for hiding this comment

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

On line 86 it appears the author intended to handle "const arrays" in the "pointee check". They are indeed handled correctly if one of the options is true (I don't recall which one). Is there a bug in that check that we should fix instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively we might want to correct/remove the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the comment means check the constness of pointer and array instead of const pointer and const array

Copy link
Contributor

Choose a reason for hiding this comment

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

All the tests (including the FP at hand) pass if you remove unless(arrayType() from line 87 (as well as the part of the comment), so I think it would be preferable to do that instead of creating a ConstArrayType.

const auto RValueReference = hasType(
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 @@ -146,7 +146,8 @@ Changes in existing checks
`AllowedTypes`, that excludes specified types from const-correctness
checking and fixing false positives when modifying variant by ``operator[]``
with template in parameters and supporting to check pointee mutation by
`AnalyzePointers` option.
`AnalyzePointers` option and fixing false positives when using const array
type.

- Improved :doc:`misc-redundant-expression
<clang-tidy/checks/misc/redundant-expression>` check by providing additional
Expand Down
8 changes: 8 additions & 0 deletions clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,3 +1007,11 @@ template <typename T> void f() {
x[T{}] = 3;
}
} // namespace gh127776_false_positive

namespace gh132931_false_positive {
using T = const int;
void valid(int i) {
const int arr0[] = {1, 2, 3};
T arr1[] = {1, 2, 3};
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we get a warning here, since arr1 is not used and can therefore be const?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry I see now T is const int 🤦‍♂️

}
} // namespace gh132931_false_positive