[PATCH] D113804: [clang-tidy] Fix behavior of `modernize-use-using` with nested structs/unions

[Fabian Wolff via Phabricator via cfe-commits]

fwolff created this revision.
fwolff added reviewers: alexfh, whisperity.
fwolff added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, rnkovacs, xazax.hun.
fwolff requested review of this revision.
Herald added a subscriber: cfe-commits.

Fixes PR#50990.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113804

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -302,3 +302,7 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
   // CHECK-FIXES: using b = InjectedClassNameWithUnnamedArgument;
 };
+
+typedef struct { int a; union { int b; }; } PR50990;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990 = struct { int a; union { int b; }; };
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_USING_H
 
 #include "../ClangTidyCheck.h"
+#include <map>
 
 namespace clang {
 namespace tidy {
@@ -23,7 +24,8 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  std::map<const Decl *, SourceRange> LastTagDeclRanges;
+
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -25,19 +25,42 @@
 }
 
 void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+                                 hasParent(decl().bind("parent-decl")))
+                         .bind("typedef"),
                      this);
-  // This matcher used to find tag declarations in source code within typedefs.
-  // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(tagDecl(unless(isImplicit())).bind("tagdecl"), this);
+
+  // This matcher is used to find tag declarations in source code within
+  // typedefs. They appear in the AST just *prior* to the typedefs.
+  Finder->addMatcher(
+      tagDecl(
+          anyOf(allOf(unless(anyOf(isImplicit(),
+                                   classTemplateSpecializationDecl())),
+                      hasParent(decl().bind("parent-decl"))),
+                // We want the parent of the ClassTemplateDecl, not the parent
+                // of the specialization.
+                classTemplateSpecializationDecl(hasAncestor(
+                    classTemplateDecl(hasParent(decl().bind("parent-decl")))))))
+          .bind("tagdecl"),
+      this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ParentDecl = Result.Nodes.getNodeAs<Decl>("parent-decl");
+  if (!ParentDecl) {
+    return;
+  }
+
   // Match CXXRecordDecl only to store the range of the last non-implicit full
   // declaration, to later check whether it's within the typdef itself.
   const auto *MatchedTagDecl = Result.Nodes.getNodeAs<TagDecl>("tagdecl");
   if (MatchedTagDecl) {
-    LastTagDeclRange = MatchedTagDecl->getSourceRange();
+    // It is not sufficient to just track the last TagDecl that we've seen,
+    // because if one struct or union is nested inside another, the last TagDecl
+    // 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();
     return;
   }
 
@@ -102,12 +125,14 @@
   auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
 
   // If typedef contains a full tag declaration, extract its full text.
-  if (LastTagDeclRange.isValid() &&
-      ReplaceRange.fullyContains(LastTagDeclRange)) {
+  auto LastTagDeclRange = LastTagDeclRanges.find(ParentDecl);
+  if (LastTagDeclRange != LastTagDeclRanges.end() &&
+      LastTagDeclRange->second.isValid() &&
+      ReplaceRange.fullyContains(LastTagDeclRange->second)) {
     bool Invalid;
-    Type = std::string(
-        Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
-                             *Result.SourceManager, getLangOpts(), &Invalid));
+    Type = std::string(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(LastTagDeclRange->second),
+        *Result.SourceManager, getLangOpts(), &Invalid));
     if (Invalid)
       return;
   }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits