-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
[clang-tidy] Improved modernize-use-using by fixing a false-negative #82947
Conversation
003ec9d
to
d1cbed0
Compare
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Félix-Antoine Constantin (felix642) ChangesThe check needs a parent decl to match but if the typedef is in a function, the parent is a declStmt which is not a decl by itself. Improved the matcher to match on either a decl or a declstmt and extract the decl from the stmt in the latter case. fixes #72179 CC: @FabianWolff @PiotrZSL @njames93 Full diff: https://github.com/llvm/llvm-project/pull/82947.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
index bb05f206c717ce..50a07fc02e31b4 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -24,6 +24,7 @@ static constexpr llvm::StringLiteral ExternCDeclName = "extern-c-decl";
static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
static constexpr llvm::StringLiteral TypedefName = "typedef";
+static constexpr llvm::StringLiteral DeclStmtName = "decl-stmt";
UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
@@ -41,7 +42,8 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
unless(isInstantiated()),
optionally(hasAncestor(
linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))),
- hasParent(decl().bind(ParentDeclName)))
+ anyOf(hasParent(decl().bind(ParentDeclName)),
+ hasParent(declStmt().bind(DeclStmtName))))
.bind(TypedefName),
this);
@@ -51,17 +53,25 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
tagDecl(
anyOf(allOf(unless(anyOf(isImplicit(),
classTemplateSpecializationDecl())),
- hasParent(decl().bind(ParentDeclName))),
+ anyOf(hasParent(decl().bind(ParentDeclName)),
+ hasParent(declStmt().bind(DeclStmtName)))),
// We want the parent of the ClassTemplateDecl, not the parent
// of the specialization.
classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
- hasParent(decl().bind(ParentDeclName)))))))
+ anyOf(hasParent(decl().bind(ParentDeclName)),
+ hasParent(declStmt().bind(DeclStmtName))))))))
.bind(TagDeclName),
this);
}
void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
const auto *ParentDecl = Result.Nodes.getNodeAs<Decl>(ParentDeclName);
+
+ if (!ParentDecl) {
+ const auto *ParentDeclStmt = Result.Nodes.getNodeAs<DeclStmt>(DeclStmtName);
+ ParentDecl = ParentDeclStmt->getSingleDecl();
+ }
+
if (!ParentDecl)
return;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6fd01ed9d471c5..2b36ae4acb9f1d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -183,6 +183,9 @@ Changes in existing checks
<clang-tidy/checks/modernize/use-override>` check to also remove any trailing
whitespace when deleting the ``virtual`` keyword.
+- Improved :doc:`modernize-use-using <clang-tidy/checks/modernize/use-using>`
+ check by fixing false-negative in functions.
+
- Improved :doc:`readability-implicit-bool-conversion
<clang-tidy/checks/readability/implicit-bool-conversion>` check to provide
valid fix suggestions for ``static_cast`` without a preceding space and
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp
index 462bc984fd3ad5..230c7c94cda1cf 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp
@@ -342,3 +342,21 @@ typedef int InExternCPP;
// CHECK-FIXES: using InExternCPP = int;
}
+
+namespace ISSUE_72179
+{
+ void foo()
+ {
+ typedef int a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use 'using' instead of 'typedef' [modernize-use-using]
+ // CHECK-FIXES: using a = int;
+
+ }
+
+ void foo2()
+ {
+ typedef struct { int a; union { int b; }; } c;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use 'using' instead of 'typedef' [modernize-use-using]
+ // CHECK-FIXES: using c = struct { int a; union { int b; }; };
+ }
+}
|
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.
Overall fine.
46bc05a
to
617925e
Compare
Thank you for the quick review @PiotrZSL. I've implemented the proposed changes and rebased on top of main. We can wait a few days to see if someone else has some comments to add and then we should be ready to merge |
1569375
to
e67122c
Compare
@PiotrZSL I've squashed my changed. When you'll have time, I think we can merge this PR. |
The check needs a parent decl to match but if the typedef is in a function, the parent is a declStmt which is not a decl by itself. Improved the matcher to match on either a decl or a declstmt and extract the decl from the stmt in the latter case. fixes llvm#72179
e67122c
to
72f2b39
Compare
✅ With the latest revision this PR passed the Python code formatter. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
The check needs a parent decl to match but if the typedef is in a function, the parent is a declStmt which is not a decl by itself. Improved the matcher to match on either a decl or a declstmt and extract the decl from the stmt in the latter case.
fixes #72179
CC: @FabianWolff @PiotrZSL @njames93