Skip to content
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]: Add TagDecl into LastTagDeclRanges in UseUsingCheck only when it is a definition #67639

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

jcsxky
Copy link
Contributor

@jcsxky jcsxky commented Sep 28, 2023

Fix issue 67529, clang-tidy: modernize-use-using fails when type is implicitly forward declared
The problem is that using Lexer to get record declaration will lose the type information when its original type is pointer or reference. This patch fix this problem by skip adding the tag declaration when it's only a 'declaration' and not a 'definition'.

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 28, 2023

@llvm/pr-subscribers-clang-tidy

Changes

Fix issue 67529, clang-tidy: modernize-use-using fails when type is implicitly forward declared


Full diff: https://github.com/llvm/llvm-project/pull/67639.diff

2 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp (+2-1)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp (+4)
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
index 22dc9e21cab9d5a..e6293ed48bfddbb 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -61,7 +61,8 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
     // before the typedef will be the nested one (PR#50990). Therefore, we also
     // keep track of the parent declaration, so that we can look up the last
     // TagDecl that is a sibling of the typedef in the AST.
-    LastTagDeclRanges[ParentDecl] = MatchedTagDecl->getSourceRange();
+    if (MatchedTagDecl->isThisDeclarationADefinition())
+      LastTagDeclRanges[ParentDecl] = MatchedTagDecl->getSourceRange();
     return;
   }
 
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 f7db0af6434ac42..422abee11a71962 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
@@ -321,3 +321,7 @@ typedef bool (*ISSUE_65055_2)(int);
 // CHECK-MESSAGES: :[[@LINE-2]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: {{^}}using ISSUE_65055_1 = void (*)(int);{{$}}
 // CHECK-FIXES: {{^}}using ISSUE_65055_2 = bool (*)(int);{{$}}
+
+typedef class ISSUE_67529_1 *ISSUE_67529;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using ISSUE_67529 = class ISSUE_67529_1 *;

@jcsxky jcsxky requested a review from a team September 28, 2023 07:42
@jcsxky jcsxky changed the title [clang-tidy]: Add TagDecl only when it is a definition && Fix issue 6… [clang-tidy]: Add TagDecl into LastTagDeclRanges in UseUsingCheck only when it is a definition Sep 28, 2023
@PiotrZSL
Copy link
Member

Release note entry would be welcome.

@jcsxky
Copy link
Contributor Author

jcsxky commented Sep 28, 2023

Release note entry would be welcome.

updated

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

Fix release notes, update change description (commit) to describe what was a problem. Mainly why isThisDeclarationADefinition is needed.
After that leave it open for few days before pushing, so someone else could also look into this and provide some comments if needed.

@jcsxky
Copy link
Contributor Author

jcsxky commented Sep 28, 2023

Fix release notes, update change description (commit) to describe what was a problem. Mainly why isThisDeclarationADefinition is needed. After that leave it open for few days before pushing, so someone else could also look into this and provide some comments if needed.

OK. Thanks for your review. Release notes and description have been updated.

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

LGTM

@jcsxky jcsxky merged commit eef35c2 into llvm:main Oct 5, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-tidy: modernize-use-using fails when type is implicitly forward declared
3 participants