-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[clang-tidy] added RespectOpaqueTypes
option to readability-qualified-auto check
#147060
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
base: main
Are you sure you want to change the base?
[clang-tidy] added RespectOpaqueTypes
option to readability-qualified-auto check
#147060
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Juan Besa (JuanBesa) Changes
A more common example is some implementations of This introduces the RespectOpaqueTypes option so that Full diff: https://github.com/llvm/llvm-project/pull/147060.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp b/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
index 91a08b9d8de69..d5d2163f83679 100644
--- a/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
@@ -106,12 +106,14 @@ QualifiedAutoCheck::QualifiedAutoCheck(StringRef Name,
: ClangTidyCheck(Name, Context),
AddConstToQualified(Options.get("AddConstToQualified", true)),
AllowedTypes(
- utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
+ utils::options::parseStringList(Options.get("AllowedTypes", ""))),
+ RespectOpaqueTypes(Options.get("RespectOpaqueTypes", false)) {}
void QualifiedAutoCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "AddConstToQualified", AddConstToQualified);
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
+ Options.store(Opts, "RespectOpaqueTypes", RespectOpaqueTypes);
}
void QualifiedAutoCheck::registerMatchers(MatchFinder *Finder) {
@@ -174,6 +176,21 @@ void QualifiedAutoCheck::registerMatchers(MatchFinder *Finder) {
void QualifiedAutoCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("auto")) {
+ if (RespectOpaqueTypes) {
+ auto DeducedType =
+ Var->getType()->getContainedAutoType()->getDeducedType();
+
+ // Remove one sugar if the type if part of a template
+ if (llvm::isa<SubstTemplateTypeParmType>(DeducedType)) {
+ DeducedType =
+ DeducedType->getLocallyUnqualifiedSingleStepDesugaredType();
+ }
+
+ if (!isa<PointerType>(DeducedType)) {
+ return;
+ }
+ }
+
SourceRange TypeSpecifier;
if (std::optional<SourceRange> TypeSpec =
getTypeSpecifierLocation(Var, Result)) {
diff --git a/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.h b/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.h
index 187a4cd6ee614..f88f7e6489538 100644
--- a/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.h
@@ -32,6 +32,7 @@ class QualifiedAutoCheck : public ClangTidyCheck {
private:
const bool AddConstToQualified;
const std::vector<StringRef> AllowedTypes;
+ const bool RespectOpaqueTypes;
};
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/qualified-auto-opaque.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/qualified-auto-opaque.cpp
new file mode 100644
index 0000000000000..a9a63663919f7
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/qualified-auto-opaque.cpp
@@ -0,0 +1,239 @@
+// RUN: %check_clang_tidy %s readability-qualified-auto %t -- -config="{CheckOptions: [\
+// RUN: {key: readability-qualified-auto.RespectOpaqueTypes, value: true}]}" --
+
+namespace typedefs {
+typedef int *MyPtr;
+typedef int &MyRef;
+typedef const int *CMyPtr;
+typedef const int &CMyRef;
+
+MyPtr getPtr();
+MyPtr* getPtrPtr();
+MyRef getRef();
+CMyPtr getCPtr();
+CMyPtr* getCPtrPtr();
+CMyRef getCRef();
+int* getIntPtr();
+
+void foo() {
+ auto TdNakedPtr = getPtr();
+ auto TdNakedPtrPtr = getPtrPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto TdNakedPtrPtr' can be declared as 'auto *TdNakedPtrPtr'
+ // CHECK-FIXES: {{^}} auto *TdNakedPtrPtr = getPtrPtr();
+ auto intPtr = getIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto intPtr' can be declared as 'auto *intPtr'
+ // CHECK-FIXES: {{^}} auto *intPtr = getIntPtr();
+ auto TdNakedRefDeref = getRef();
+ auto TdNakedCPtr = getCPtr();
+ auto TdNakedCPtrPtr = getCPtrPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto TdNakedCPtrPtr' can be declared as 'auto *TdNakedCPtrPtr'
+ // CHECK-FIXES: {{^}} auto *TdNakedCPtrPtr = getCPtrPtr();
+ auto &TdNakedCRef = getCRef();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto &TdNakedCRef' can be declared as 'const auto &TdNakedCRef'
+ // CHECK-FIXES: {{^}} const auto &TdNakedCRef = getCRef();
+ auto TdNakedCRefDeref = getCRef();
+}
+
+}; // namespace typedefs
+
+namespace usings {
+using MyPtr = int *;
+using MyRef = int &;
+using CMyPtr = const int *;
+using CMyRef = const int &;
+
+MyPtr getPtr();
+MyPtr* getPtrPtr();
+MyRef getRef();
+CMyPtr getCPtr();
+CMyRef getCRef();
+
+void foo() {
+ auto UNakedPtr = getPtr();
+ auto UNakedPtrPtr = getPtrPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto UNakedPtrPtr' can be declared as 'auto *UNakedPtrPtr'
+ // CHECK-FIXES: {{^}} auto *UNakedPtrPtr = getPtrPtr();
+ auto &UNakedRef = getRef();
+ auto UNakedRefDeref = getRef();
+ auto UNakedCPtr = getCPtr();
+ auto &UNakedCRef = getCRef();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto &UNakedCRef' can be declared as 'const auto &UNakedCRef'
+ // CHECK-FIXES: {{^}} const auto &UNakedCRef = getCRef();
+ auto UNakedCRefDeref = getCRef();
+}
+
+}; // namespace usings
+
+int *getIntPtr();
+const int *getCIntPtr();
+
+void foo() {
+ // make sure check disregards named types
+ int *TypedPtr = getIntPtr();
+ const int *TypedConstPtr = getCIntPtr();
+ int &TypedRef = *getIntPtr();
+ const int &TypedConstRef = *getCIntPtr();
+
+ auto NakedPtr = getIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto NakedPtr' can be declared as 'auto *NakedPtr'
+ // CHECK-FIXES: {{^}} auto *NakedPtr = getIntPtr();
+ auto NakedCPtr = getCIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto NakedCPtr' can be declared as 'const auto *NakedCPtr'
+ // CHECK-FIXES: {{^}} const auto *NakedCPtr = getCIntPtr();
+
+ const auto ConstPtr = getIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'const auto ConstPtr' can be declared as 'auto *const ConstPtr'
+ // CHECK-FIXES: {{^}} auto *const ConstPtr = getIntPtr();
+ const auto ConstCPtr = getCIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'const auto ConstCPtr' can be declared as 'const auto *const ConstCPtr'
+ // CHECK-FIXES: {{^}} const auto *const ConstCPtr = getCIntPtr();
+
+ volatile auto VolatilePtr = getIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'volatile auto VolatilePtr' can be declared as 'auto *volatile VolatilePtr'
+ // CHECK-FIXES: {{^}} auto *volatile VolatilePtr = getIntPtr();
+ volatile auto VolatileCPtr = getCIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'volatile auto VolatileCPtr' can be declared as 'const auto *volatile VolatileCPtr'
+ // CHECK-FIXES: {{^}} const auto *volatile VolatileCPtr = getCIntPtr();
+
+ auto *QualPtr = getIntPtr();
+ auto *QualCPtr = getCIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto *QualCPtr' can be declared as 'const auto *QualCPtr'
+ // CHECK-FIXES: {{^}} const auto *QualCPtr = getCIntPtr();
+ auto *const ConstantQualCPtr = getCIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto *const ConstantQualCPtr' can be declared as 'const auto *const ConstantQualCPtr'
+ // CHECK-FIXES: {{^}} const auto *const ConstantQualCPtr = getCIntPtr();
+ auto *volatile VolatileQualCPtr = getCIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto *volatile VolatileQualCPtr' can be declared as 'const auto *volatile VolatileQualCPtr'
+ // CHECK-FIXES: {{^}} const auto *volatile VolatileQualCPtr = getCIntPtr();
+ const auto *ConstQualCPtr = getCIntPtr();
+
+ auto &Ref = *getIntPtr();
+ auto &CRef = *getCIntPtr();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto &CRef' can be declared as 'const auto &CRef'
+ // CHECK-FIXES: {{^}} const auto &CRef = *getCIntPtr();
+ const auto &ConstCRef = *getCIntPtr();
+
+ if (auto X = getCIntPtr()) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'auto X' can be declared as 'const auto *X'
+ // CHECK-FIXES: {{^}} if (const auto *X = getCIntPtr()) {
+ }
+}
+
+namespace std {
+
+template <typename T>
+class vector { // dummy impl
+ T _data[1];
+
+public:
+ T *begin() { return _data; }
+ const T *begin() const { return _data; }
+ T *end() { return &_data[1]; }
+ const T *end() const { return &_data[1]; }
+};
+
+
+} // namespace std
+
+namespace loops {
+
+void change(int &);
+void observe(const int &);
+
+void loopRef(std::vector<int> &Mutate, const std::vector<int> &Constant) {
+ for (auto &Data : Mutate) {
+ change(Data);
+ }
+ for (auto &Data : Constant) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'auto &Data' can be declared as 'const auto &Data'
+ // CHECK-FIXES: {{^}} for (const auto &Data : Constant) {
+ observe(Data);
+ }
+}
+
+void loopPtr(const std::vector<int *> &Mutate, const std::vector<const int *> &Constant) {
+ for (auto Data : Mutate) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'auto Data' can be declared as 'auto *Data'
+ // CHECK-FIXES: {{^}} for (auto *Data : Mutate) {
+ change(*Data);
+ }
+ for (auto Data : Constant) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'auto Data' can be declared as 'const auto *Data'
+ // CHECK-FIXES: {{^}} for (const auto *Data : Constant) {
+ observe(*Data);
+ }
+}
+
+template <typename T>
+void tempLoopPtr(std::vector<T *> &MutateTemplate, std::vector<const T *> &ConstantTemplate) {
+ for (auto Data : MutateTemplate) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'auto Data' can be declared as 'auto *Data'
+ // CHECK-FIXES: {{^}} for (auto *Data : MutateTemplate) {
+ change(*Data);
+ }
+ //FixMe
+ for (auto Data : ConstantTemplate) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'auto Data' can be declared as 'const auto *Data'
+ // CHECK-FIXES: {{^}} for (const auto *Data : ConstantTemplate) {
+ observe(*Data);
+ }
+}
+
+template <typename T>
+class TemplateLoopPtr {
+public:
+ void operator()(const std::vector<T *> &MClassTemplate, const std::vector<const T *> &CClassTemplate) {
+ for (auto Data : MClassTemplate) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'auto Data' can be declared as 'auto *Data'
+ // CHECK-FIXES: {{^}} for (auto *Data : MClassTemplate) {
+ change(*Data);
+ }
+ //FixMe
+ for (auto Data : CClassTemplate) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'auto Data' can be declared as 'const auto *Data'
+ // CHECK-FIXES: {{^}} for (const auto *Data : CClassTemplate) {
+ observe(*Data);
+ }
+ }
+};
+
+void bar() {
+ std::vector<int> Vec;
+ std::vector<int *> PtrVec;
+ std::vector<const int *> CPtrVec;
+ loopRef(Vec, Vec);
+ loopPtr(PtrVec, CPtrVec);
+ tempLoopPtr(PtrVec, CPtrVec);
+ TemplateLoopPtr<int>()(PtrVec, CPtrVec);
+}
+
+typedef int *(*functionRetPtr)();
+typedef int (*functionRetVal)();
+
+functionRetPtr getPtrFunction();
+functionRetVal getValFunction();
+
+void baz() {
+ auto MyFunctionPtr = getPtrFunction();
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto MyFunctionPtr' can be declared as 'auto *MyFunctionPtr'
+ // CHECK-FIXES-NOT: {{^}} auto *MyFunctionPtr = getPtrFunction();
+ auto MyFunctionVal = getValFunction();
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto MyFunctionVal' can be declared as 'auto *MyFunctionVal'
+ // CHECK-FIXES-NOT: {{^}} auto *MyFunctionVal = getValFunction();
+
+ auto LambdaTest = [] { return 0; };
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto LambdaTest' can be declared as 'auto *LambdaTest'
+ // CHECK-FIXES-NOT: {{^}} auto *LambdaTest = [] { return 0; };
+
+ auto LambdaTest2 = +[] { return 0; };
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto LambdaTest2' can be declared as 'auto *LambdaTest2'
+ // CHECK-FIXES-NOT: {{^}} auto *LambdaTest2 = +[] { return 0; };
+
+ auto MyFunctionRef = *getPtrFunction();
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto MyFunctionRef' can be declared as 'auto *MyFunctionRef'
+ // CHECK-FIXES-NOT: {{^}} auto *MyFunctionRef = *getPtrFunction();
+
+ auto &MyFunctionRef2 = *getPtrFunction();
+}
+
+} // namespace loops
|
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.
Please mention changes in Release Notes.
@@ -174,6 +176,21 @@ void QualifiedAutoCheck::registerMatchers(MatchFinder *Finder) { | |||
|
|||
void QualifiedAutoCheck::check(const MatchFinder::MatchResult &Result) { | |||
if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("auto")) { | |||
if (RespectOpaqueTypes) { | |||
auto DeducedType = |
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.
Please do not use auto
if type is not spelled explicitly in same statement or iterator.
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.
Please, describe your option in check docs.
I find option name a little unclear, I don't encounter "opaque" term ofter, maybe DesugarTypes
or AllowSugaredTypes
are better? WDYT reviewers?
UPD. I think IgnoreAliasing
from 5chmidti is the best name.
@@ -0,0 +1,239 @@ | |||
// RUN: %check_clang_tidy %s readability-qualified-auto %t -- -config="{CheckOptions: [\ |
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.
Please write tests in existing qualified-auto.cpp
. You can have multiple RUN commands in one file.
The option name could also be something like |
I like |
Thanks for the comments. I also think I'll look into merging the tests into a single file, and see if I can figure out the matcher. |
Yes, that would be just as current default behavior. |
@@ -347,6 +347,11 @@ Changes in existing checks | |||
<clang-tidy/checks/readability/redundant-smartptr-get>` check by fixing | |||
some false positives involving smart pointers to arrays. | |||
|
|||
- Improved :doc:`readability-qualified-auto |
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.
Please merge with existing readability-qualified-auto
entry.
readability-qualified-auto
check currently looks at the unsugared type, skipping any typedefs, to determine if the variable is a pointer-type. This may not be the desired behaviour, in particular when the type depends on compilation flags.For example
A more common example is some implementations of
std::array
use pointers as iterators.This introduces the RespectOpaqueTypes option so that
readability-qualified-auto
does not look beyond typedefs.