-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang-tidy] add AllowedTypes option to misc-const-correctness #122951
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.
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 9 commits into
llvm:main
from
vbvictor:misc-const-correctness-allowed-types-opt
Feb 19, 2025
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
899a4d3
[clang-tidy] add AllowedTypes to misc-const-correctness
vbvictor 50e5241
[clang-tidy] fix pr comments
vbvictor 8cad29a
[clang-tidy] fix pr comments
vbvictor ed89910
[clang-tidy] fix pr comments
vbvictor b81904f
[clang-tidy] improved docs by filling lines to 80-character limit
vbvictor 260554f
[clang-tidy] fix pr docs comments
vbvictor 2dc205b
[clang-tidy] fixed styles of default parameters in docs
vbvictor 8e514d6
[clang-tidy] fixed pr comments
vbvictor 24d12e6
[clang-tidy] Added double ticks
vbvictor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
180 changes: 180 additions & 0 deletions
180
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-allowed-types.cpp
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,180 @@ | ||
// RUN: %check_clang_tidy %s misc-const-correctness %t -- \ | ||
// RUN: -config="{CheckOptions: {\ | ||
// RUN: misc-const-correctness.AllowedTypes: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$;qualified::Type;::fully::QualifiedType;ConstTemplate', \ | ||
// RUN: misc-const-correctness.TransformPointersAsValues: true, \ | ||
// RUN: misc-const-correctness.TransformReferences: true, \ | ||
// RUN: misc-const-correctness.WarnPointersAsValues: true } \ | ||
// RUN: }" -- -fno-delayed-template-parsing | ||
|
||
struct SmartPointer { | ||
}; | ||
|
||
struct smart_pointer { | ||
}; | ||
|
||
struct SmartPtr { | ||
}; | ||
|
||
struct smart_ptr { | ||
}; | ||
|
||
struct SmartReference { | ||
}; | ||
|
||
struct smart_reference { | ||
}; | ||
|
||
struct SmartRef { | ||
}; | ||
|
||
struct smart_ref { | ||
}; | ||
|
||
struct OtherType { | ||
}; | ||
|
||
template <typename T> struct ConstTemplate { | ||
}; | ||
|
||
namespace qualified { | ||
struct Type { | ||
}; | ||
} // namespace qualified | ||
|
||
namespace fully { | ||
struct QualifiedType { | ||
}; | ||
} // namespace fully | ||
|
||
void negativeSmartPointer() { | ||
SmartPointer p1 = {}; | ||
SmartPointer* p2 = {}; | ||
SmartPointer& p3 = p1; | ||
} | ||
|
||
void negative_smart_pointer() { | ||
smart_pointer p1 = {}; | ||
smart_pointer* p2 = {}; | ||
smart_pointer& p3 = p1; | ||
} | ||
|
||
void negativeSmartPtr() { | ||
SmartPtr p1 = {}; | ||
SmartPtr* p2 = {}; | ||
SmartPtr& p3 = p1; | ||
} | ||
|
||
void negative_smart_ptr() { | ||
smart_ptr p1 = {}; | ||
smart_ptr* p2 = {}; | ||
smart_ptr& p3 = p1; | ||
} | ||
|
||
void negativeSmartReference() { | ||
SmartReference p1 = {}; | ||
SmartReference* p2 = {}; | ||
SmartReference& p3 = p1; | ||
} | ||
|
||
void negative_smart_reference() { | ||
smart_reference p1 = {}; | ||
smart_reference* p2 = {}; | ||
smart_reference& p3 = p1; | ||
} | ||
|
||
void negativeSmartRef() { | ||
SmartRef p1 = {}; | ||
SmartRef* p2 = {}; | ||
SmartRef& p3 = p1; | ||
} | ||
|
||
void negative_smart_ref() { | ||
smart_ref p1 = {}; | ||
smart_ref* p2 = {}; | ||
smart_ref& p3 = p1; | ||
} | ||
|
||
void positiveOtherType() { | ||
OtherType t = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't' of type 'OtherType' can be declared 'const' | ||
// CHECK-FIXES: OtherType const t = {}; | ||
} | ||
|
||
void negativeSomeComplex() { | ||
ConstTemplate<int> t1 = {}; | ||
ConstTemplate<int>* t2 = {}; | ||
ConstTemplate<int>& t3 = t1; | ||
} | ||
|
||
void negativeQualified() { | ||
qualified::Type t1 = {}; | ||
qualified::Type* t2 = {}; | ||
qualified::Type& t3 = t1; | ||
|
||
using qualified::Type; | ||
Type t4 = {}; | ||
Type* t5 = {}; | ||
Type& t6 = t4; | ||
} | ||
|
||
void negativeFullyQualified() { | ||
fully::QualifiedType t1 = {}; | ||
fully::QualifiedType* t2 = {}; | ||
fully::QualifiedType& t3 = t1; | ||
|
||
using fully::QualifiedType; | ||
QualifiedType t4 = {}; | ||
QualifiedType* t5 = {}; | ||
QualifiedType& t6 = t4; | ||
} | ||
|
||
using MySP = SmartPointer; | ||
using MyTemplate = ConstTemplate<int>; | ||
template <typename T> using MyTemplate2 = ConstTemplate<T>; | ||
|
||
void positiveTypedefs() { | ||
MySP p1 = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p1' of type 'MySP' (aka 'SmartPointer') can be declared 'const' | ||
// CHECK-FIXES: MySP const p1 = {}; | ||
|
||
MySP* p2 = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p2' of type 'MySP *' (aka 'SmartPointer *') can be declared 'const' | ||
// CHECK-FIXES: MySP* const p2 = {}; | ||
|
||
MySP& p3 = p1; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p3' of type 'MySP &' (aka 'SmartPointer &') can be declared 'const' | ||
// CHECK-FIXES: MySP const& p3 = p1; | ||
|
||
MyTemplate t1 = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't1' of type 'MyTemplate' (aka 'ConstTemplate<int>') can be declared 'const' | ||
// CHECK-FIXES: MyTemplate const t1 = {}; | ||
|
||
MyTemplate* t2 = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't2' of type 'MyTemplate *' (aka 'ConstTemplate<int> *') can be declared 'const' | ||
// CHECK-FIXES: MyTemplate* const t2 = {}; | ||
|
||
MyTemplate& t3 = t1; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't3' of type 'MyTemplate &' (aka 'ConstTemplate<int> &') can be declared 'const' | ||
// CHECK-FIXES: MyTemplate const& t3 = t1; | ||
|
||
MyTemplate2<int> t4 = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't4' of type 'MyTemplate2<int>' (aka 'ConstTemplate<int>') can be declared 'const' | ||
// CHECK-FIXES: MyTemplate2<int> const t4 = {}; | ||
|
||
MyTemplate2<int>* t5 = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't5' of type 'MyTemplate2<int> *' (aka 'ConstTemplate<int> *') can be declared 'const' | ||
// CHECK-FIXES: MyTemplate2<int>* const t5 = {}; | ||
|
||
MyTemplate2<int>& t6 = t4; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't6' of type 'MyTemplate2<int> &' (aka 'ConstTemplate<int> &') can be declared 'const' | ||
// CHECK-FIXES: MyTemplate2<int> const& t6 = t4; | ||
} | ||
|
||
template <typename T> | ||
class Vector {}; | ||
|
||
void positiveSmartPtrWrapped() { | ||
Vector<SmartPtr> vec = {}; | ||
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'vec' of type 'Vector<SmartPtr>' can be declared 'const' | ||
// CHECK-FIXES: Vector<SmartPtr> const vec = {}; | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the only tests for using declarations, for this checker in the entire test suite.
Is a negative match really what is wanted here, or is this just testing the current behavior, and this missed adding a FIXME?
CC: @HerrCai0907
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I generally tested that types placed inside
AlloweTypes
option are not matched (they arequalified::Type;::fully::QualifiedType;
).I think that
should be equivalent to
using fully::QualifiedType; QualifiedType t1 = {};
thus both are not matched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other checker tests I think we could add using declarations, but can it happen that
fully::QualifiedType t1
is matched butQualifiedType t1
isn't?Note that we check for sugared types
llvm-project/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
Lines 15 to 20 in fb761aa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
Should that be allowed or not?
In other words, for a UsingType, do we disregard how that using type was spelled, and only look at its underlying type?
Another example:
Should this disregard all UsingTypes recursively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we usually do that in clang-tidy. Here is a piece of relevant code,
Regex
here contains one of the entries ofAllowedTypes
. And the whole matcher for allowing type isqualType(hasDeclaration(namedDecl(matchers::matchesAnyListedName(AllowedTypes))))
.Here I purposly didn't use
hasCanonicalType
orhasUnqualifiedDesugaredType
to allow users to specify aliased types directly inAllowedType
, e.g.using Mutex = std::mutex
withAllowedTypes = Mutex
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this, I mean that usually clang-tidy checks have
hasUnqualifiedDesugaredType
orhasCanonicalType
to match some type, so we disregard all UsingTypes.