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

[Fabian Wolff via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG95d77383f2ba: [clang-tidy] Fix behavior of 
`modernize-use-using` with nested structs/unions (authored by fwolff).

Changed prior to commit:
  https://reviews.llvm.org/D113804?vs=400677=424172#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

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,15 @@
   // 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; }; };
+
+typedef struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; } PR50990_nested;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_nested = struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; };
+
+typedef struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; } PR50990_siblings;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_siblings = struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; };
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
@@ -23,7 +23,8 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  llvm::DenseMap 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
@@ -16,6 +16,10 @@
 namespace tidy {
 namespace modernize {
 
+static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
+static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
+static constexpr llvm::StringLiteral TypedefName = "typedef";
+
 UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
@@ -25,23 +29,45 @@
 }
 
 void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+ hasParent(decl().bind(ParentDeclName)))
+ .bind(TypedefName),
  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(ParentDeclName))),
+// We want the parent of the ClassTemplateDecl, not the parent
+// of the specialization.
+classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
+hasParent(decl().bind(ParentDeclName)))
+  .bind(TagDeclName),
+  this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult ) {
+  const auto *ParentDecl = Result.Nodes.getNodeAs(ParentDeclName);
+  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");
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs(TagDeclName);
   if (MatchedTagDecl) {
-LastTagDeclRange = MatchedTagDecl->getSourceRange();
+// It is not sufficient to just track the last TagDecl that 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a nit.




Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:39-48
+  Finder->addMatcher(
+  tagDecl(
+  anyOf(allOf(unless(anyOf(isImplicit(),
+   classTemplateSpecializationDecl())),
+  hasParent(decl().bind(ParentDeclName))),
+// We want the parent of the ClassTemplateDecl, not the parent
+// of the specialization.

I recall that using `hasParent()` and `hasAncestor()` tend to be expensive 
because they walk up the entire AST to the TU level, and so they can also match 
in surprising places. However, I can't seem to devise a test case that will 
fail, so I think this is probably okay.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp:306
+
+// clang-format off
+

We don't typically use clang-format comments (especially not in tests; those 
are almost never formatted anyway). Same below.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

https://reviews.llvm.org/D113804

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

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

[Fabian Wolff via Phabricator via cfe-commits]

fwolff added a comment.

Ping @aaron.ballman.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

https://reviews.llvm.org/D113804

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

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

[Fabian Wolff via Phabricator via cfe-commits]

fwolff updated this revision to Diff 400677.
fwolff added a comment.

Rebased and ping @whisperity


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

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,19 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
   // CHECK-FIXES: using b = InjectedClassNameWithUnnamedArgument;
 };
+
+// clang-format off
+
+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; }; };
+
+typedef struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; } PR50990_nested;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_nested = struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; };
+
+typedef struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; } PR50990_siblings;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_siblings = struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; };
+
+// clang-format on
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
@@ -23,7 +23,8 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  llvm::DenseMap 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
@@ -16,6 +16,10 @@
 namespace tidy {
 namespace modernize {
 
+static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
+static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
+static constexpr llvm::StringLiteral TypedefName = "typedef";
+
 UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
@@ -25,23 +29,45 @@
 }
 
 void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+ hasParent(decl().bind(ParentDeclName)))
+ .bind(TypedefName),
  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(ParentDeclName))),
+// We want the parent of the ClassTemplateDecl, not the parent
+// of the specialization.
+classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
+hasParent(decl().bind(ParentDeclName)))
+  .bind(TagDeclName),
+  this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult ) {
+  const auto *ParentDecl = Result.Nodes.getNodeAs(ParentDeclName);
+  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");
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs(TagDeclName);
   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
+// 

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

[Fabian Wolff via Phabricator via cfe-commits]

fwolff updated this revision to Diff 391852.
fwolff added a comment.

Thanks for your comments @whisperity! They should all be fixed now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

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,15 @@
   // 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; }; };
+
+typedef struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; } PR50990_nested;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_nested = struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; };
+
+typedef struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; } PR50990_siblings;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_siblings = struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; };
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
@@ -23,7 +23,8 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  llvm::DenseMap 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
@@ -16,6 +16,10 @@
 namespace tidy {
 namespace modernize {
 
+static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
+static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
+static constexpr llvm::StringLiteral TypedefName = "typedef";
+
 UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
@@ -25,23 +29,45 @@
 }
 
 void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+ hasParent(decl().bind(ParentDeclName)))
+ .bind(TypedefName),
  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(ParentDeclName))),
+// We want the parent of the ClassTemplateDecl, not the parent
+// of the specialization.
+classTemplateSpecializationDecl(hasAncestor(
+classTemplateDecl(hasParent(decl().bind(ParentDeclName)))
+  .bind(TagDeclName),
+  this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult ) {
+  const auto *ParentDecl = Result.Nodes.getNodeAs(ParentDeclName);
+  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");
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs(TagDeclName);
   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 

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

`hasParent()` is the //direct// upwards traversal, right? I remember some 
discussion about matchers like that being potentially costly. But I can't find 
any description of this, so I guess it's fine, rewriting the matcher to have 
the opposite logic 
(`decl(hasDescendant(...the-real-matcher-here...)).bind("blah")`) would just 
make everything ugly for no tangible benefit.




Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:29
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+ hasParent(decl().bind("parent-decl")))
+ .bind("typedef"),

(Nit: Once we have multiple uses of an identifier, it's better to hoist it to a 
definition and reuse it that way, to prevent future typos introducing arcane 
issues. Generally, a `static constexpr llvm::StringLiteral` in the TU's global 
scope, is sufficient for that.)



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:50-52
+  if (!ParentDecl) {
+return;
+  }

(Style: Elid braces.)



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:132-137
 bool Invalid;
-Type = std::string(
-Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
- *Result.SourceManager, getLangOpts(), ));
+Type = std::string(Lexer::getSourceText(
+CharSourceRange::getTokenRange(LastTagDeclRange->second),
+*Result.SourceManager, getLangOpts(), ));
 if (Invalid)
   return;

(Nit: if the source ranges are invalid, a default constructed `StringRef` is 
returned, which is empty and converts to a default-constructed `std::string`.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

https://reviews.llvm.org/D113804

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

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

[Fabian Wolff via Phabricator via cfe-commits]

fwolff updated this revision to Diff 387377.
fwolff added a comment.

Thanks for your suggestions @whisperity! I have fixed both of them now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

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,15 @@
   // 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; }; };
+
+typedef struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; } PR50990_nested;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_nested = struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; };
+
+typedef struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; } PR50990_siblings;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_siblings = struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; };
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
@@ -23,7 +23,8 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  llvm::DenseMap 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 ) {
+  const auto *ParentDecl = Result.Nodes.getNodeAs("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");
   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 = 

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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h:27
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  std::map LastTagDeclRanges;
+

Considering the pointer is just a number that hashes well, is there a reason to 
use the plain `std::map`? It is [[ 
https://llvm.org/docs/ProgrammersManual.html#map | generally discouraged ]].



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp:305-308
+
+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; }; };

Just for the sake of testing a new logic, it would be beneficial to add a "more 
complex" nested example. At least one example where there are two sibling 
nests, and one where there is an additional level of nesting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113804/new/

https://reviews.llvm.org/D113804

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

[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 
 
 namespace clang {
 namespace tidy {
@@ -23,7 +24,8 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  std::map 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 ) {
+  const auto *ParentDecl = Result.Nodes.getNodeAs("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");
   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(), ));
+Type =