[clang-tools-extra] 73d9361 - [clang-tidy] modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef
Mitchell Balan via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 7 13:36:17 PST 2020
Author: Mitchell Balan
Date: 2020-01-07T16:36:11-05:00
New Revision: 73d93617d3ae23bad232fa3a160c638728c71c01
URL: https://github.com/llvm/llvm-project/commit/73d93617d3ae23bad232fa3a160c638728c71c01
DIFF: https://github.com/llvm/llvm-project/commit/73d93617d3ae23bad232fa3a160c638728c71c01.diff
LOG: [clang-tidy] modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef
Summary:
It now handles `typedef`s that include comma-separated multiple types, and handles embedded struct definitions, which previously could not be automatically converted.
For example, with this patch `modernize-use-using` now can convert:
typedef struct { int a; } R_t, *R_p;
to:
using R_t = struct { int a; };
using R_p = R_t*;
`-ast-dump` showed that the `CXXRecordDecl` definitions and multiple `TypedefDecl`s come consecutively in the tree, so `check()` stores information between calls to determine when it is receiving a second or additional `TypedefDecl` within a single `typedef`, or when the current `TypedefDecl` refers to an embedded `CXXRecordDecl` like a `struct`.
Reviewers: alexfh, aaron.ballman
Patch by: poelmanc
Subscribers: riccibruno, sammccall, cfe-commits, aaron.ballman
Tags: clang-tools-extra, clang
Differential Revision: https://reviews.llvm.org/D70270
Added:
Modified:
clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/modernize-use-using.rst
clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
clang/include/clang/Basic/SourceLocation.h
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
index c9ca8a6114a5..ce8d00f358ab 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -25,107 +25,88 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
return;
Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
this);
+ // This matcher used to find structs defined in source code within typedefs.
+ // They appear in the AST just *prior* to the typedefs.
+ Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
}
-// Checks if 'typedef' keyword can be removed - we do it only if
-// it is the only declaration in a declaration chain.
-static bool CheckRemoval(SourceManager &SM, SourceLocation StartLoc,
- ASTContext &Context) {
- assert(StartLoc.isFileID() && "StartLoc must not be in a macro");
- std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(StartLoc);
- StringRef File = SM.getBufferData(LocInfo.first);
- const char *TokenBegin = File.data() + LocInfo.second;
- Lexer DeclLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
- File.begin(), TokenBegin, File.end());
-
- Token Tok;
- int NestingLevel = 0; // Parens, braces, and square brackets
- int AngleBracketLevel = 0;
- bool FoundTypedef = false;
-
- while (!DeclLexer.LexFromRawLexer(Tok) && !Tok.is(tok::semi)) {
- switch (Tok.getKind()) {
- case tok::l_brace:
- if (NestingLevel == 0 && AngleBracketLevel == 0) {
- // At top level, this might be the `typedef struct {...} T;` case.
- // Inside parens, square brackets, or angle brackets it's not.
- return false;
- }
- ++NestingLevel;
- break;
- case tok::l_paren:
- case tok::l_square:
- ++NestingLevel;
- break;
- case tok::r_brace:
- case tok::r_paren:
- case tok::r_square:
- --NestingLevel;
- break;
- case tok::less:
- // If not nested in paren/brace/square bracket, treat as opening angle bracket.
- if (NestingLevel == 0)
- ++AngleBracketLevel;
- break;
- case tok::greater:
- // Per C++ 17 Draft N4659, Section 17.2/3
- // https://timsong-cpp.github.io/cppwp/n4659/temp.names#3:
- // "When parsing a template-argument-list, the first non-nested > is
- // taken as the ending delimiter rather than a greater-than operator."
- // If not nested in paren/brace/square bracket, treat as closing angle bracket.
- if (NestingLevel == 0)
- --AngleBracketLevel;
- break;
- case tok::comma:
- if (NestingLevel == 0 && AngleBracketLevel == 0) {
- // If there is a non-nested comma we have two or more declarations in this chain.
- return false;
- }
- break;
- case tok::raw_identifier:
- if (Tok.getRawIdentifier() == "typedef") {
- FoundTypedef = true;
- }
- break;
- default:
- break;
- }
+void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
+ // 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 *MatchedCxxRecordDecl =
+ Result.Nodes.getNodeAs<CXXRecordDecl>("struct");
+ if (MatchedCxxRecordDecl) {
+ LastCxxDeclRange = MatchedCxxRecordDecl->getSourceRange();
+ return;
}
- // Sanity check against weird macro cases.
- return FoundTypedef;
-}
-
-void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>("typedef");
if (MatchedDecl->getLocation().isInvalid())
return;
- auto &Context = *Result.Context;
- auto &SM = *Result.SourceManager;
-
SourceLocation StartLoc = MatchedDecl->getBeginLoc();
if (StartLoc.isMacroID() && IgnoreMacros)
return;
- auto Diag = diag(StartLoc, "use 'using' instead of 'typedef'");
+ static const char *UseUsingWarning = "use 'using' instead of 'typedef'";
- // do not fix if there is macro or array
- if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID())
+ // Warn at StartLoc but do not fix if there is macro or array.
+ if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
+ diag(StartLoc, UseUsingWarning);
return;
+ }
- if (CheckRemoval(SM, StartLoc, Context)) {
- auto printPolicy = PrintingPolicy(getLangOpts());
- printPolicy.SuppressScope = true;
- printPolicy.ConstantArraySizeAsWritten = true;
- printPolicy.UseVoidForZeroParams = false;
-
- Diag << FixItHint::CreateReplacement(
- MatchedDecl->getSourceRange(),
- "using " + MatchedDecl->getNameAsString() + " = " +
- MatchedDecl->getUnderlyingType().getAsString(printPolicy));
+ auto printPolicy = PrintingPolicy(getLangOpts());
+ printPolicy.SuppressScope = true;
+ printPolicy.ConstantArraySizeAsWritten = true;
+ printPolicy.UseVoidForZeroParams = false;
+
+ std::string Type = MatchedDecl->getUnderlyingType().getAsString(printPolicy);
+ std::string Name = MatchedDecl->getNameAsString();
+ SourceRange ReplaceRange = MatchedDecl->getSourceRange();
+
+ // typedefs with multiple comma-separated definitions produce multiple
+ // consecutive TypedefDecl nodes whose SourceRanges overlap. Each range starts
+ // at the "typedef" and then continues *across* previous definitions through
+ // the end of the current TypedefDecl definition.
+ std::string Using = "using ";
+ if (ReplaceRange.getBegin().isMacroID() ||
+ ReplaceRange.getBegin() >= LastReplacementEnd) {
+ // This is the first (and possibly the only) TypedefDecl in a typedef. Save
+ // Type and Name in case we find subsequent TypedefDecl's in this typedef.
+ FirstTypedefType = Type;
+ FirstTypedefName = Name;
+ } else {
+ // This is additional TypedefDecl in a comma-separated typedef declaration.
+ // Start replacement *after* prior replacement and separate with semicolon.
+ ReplaceRange.setBegin(LastReplacementEnd);
+ Using = ";\nusing ";
+
+ // If this additional TypedefDecl's Type starts with the first TypedefDecl's
+ // type, make this using statement refer back to the first type, e.g. make
+ // "typedef int Foo, *Foo_p;" -> "using Foo = int;\nusing Foo_p = Foo*;"
+ if (Type.size() > FirstTypedefType.size() &&
+ Type.substr(0, FirstTypedefType.size()) == FirstTypedefType)
+ Type = FirstTypedefName + Type.substr(FirstTypedefType.size() + 1);
}
+ if (!ReplaceRange.getEnd().isMacroID())
+ LastReplacementEnd = ReplaceRange.getEnd().getLocWithOffset(Name.size());
+
+ auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
+
+ // If typedef contains a full struct/class declaration, extract its full text.
+ if (LastCxxDeclRange.isValid() && ReplaceRange.fullyContains(LastCxxDeclRange)) {
+ bool Invalid;
+ Type =
+ Lexer::getSourceText(CharSourceRange::getTokenRange(LastCxxDeclRange),
+ *Result.SourceManager, getLangOpts(), &Invalid);
+ if (Invalid)
+ return;
+ }
+
+ std::string Replacement = Using + Name + " = " + Type;
+ Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
}
} // namespace modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
index ea11609d901b..f1899da7124b 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -22,6 +22,10 @@ namespace modernize {
class UseUsingCheck : public ClangTidyCheck {
const bool IgnoreMacros;
+ SourceLocation LastReplacementEnd;
+ SourceRange LastCxxDeclRange;
+ std::string FirstTypedefType;
+ std::string FirstTypedefName;
public:
UseUsingCheck(StringRef Name, ClangTidyContext *Context);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 0f7663932ec2..86cdbf5e8b1a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -166,6 +166,10 @@ Improvements to clang-tidy
Finds types that could be made trivially-destructible by removing out-of-line
defaulted destructor declarations.
+- The :doc:`modernize-use-using
+ <clang-tidy/checks/modernize-use-using>` check now converts typedefs containing
+ struct definitions and multiple comma-separated types.
+
- Improved :doc:`bugprone-posix-return
<clang-tidy/checks/bugprone-posix-return>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize-use-using.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize-use-using.rst
index 1456a91f3735..24c1c2b5c277 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/modernize-use-using.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize-use-using.rst
@@ -14,6 +14,8 @@ Before:
class Class{};
typedef void (Class::* MyPtrType)() const;
+ typedef struct { int a; } R_t, *R_p;
+
After:
.. code-block:: c++
@@ -23,6 +25,9 @@ After:
class Class{};
using MyPtrType = void (Class::*)() const;
+ using R_t = struct { int a; };
+ using R_p = R_t*;
+
This check requires using C++11 or higher to run.
Options
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 73fd2701bbdf..843f8943ba30 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
@@ -84,7 +84,11 @@ typedef int* PInt;
typedef int bla1, bla2, bla3;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef int bla1, bla2, bla3;
+// CHECK-MESSAGES: :[[@LINE-2]]:17: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-3]]:23: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using bla1 = int;
+// CHECK-FIXES-NEXT: using bla2 = int;
+// CHECK-FIXES-NEXT: using bla3 = int;
#define CODE typedef int INT
@@ -136,16 +140,16 @@ int typedef Bax;
typedef struct Q1 { int a; } S1;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef struct Q1 { int a; } S1;
+// CHECK-FIXES: using S1 = struct Q1 { int a; };
typedef struct { int b; } S2;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef struct { int b; } S2;
+// CHECK-FIXES: using S2 = struct { int b; };
struct Q2 { int c; } typedef S3;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: struct Q2 { int c; } typedef S3;
+// CHECK-FIXES: using S3 = struct Q2 { int c; };
struct { int d; } typedef S4;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: struct { int d; } typedef S4;
+// CHECK-FIXES: using S4 = struct { int d; };
namespace my_space {
class my_cclass {};
@@ -196,11 +200,15 @@ struct S {};
typedef S<(0 > 0), int> S_t, *S_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef S<(0 > 0), int> S_t, *S_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:28: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using S_t = S<(0 > 0), int>;
+// CHECK-FIXES-NEXT: using S_p = S_t*;
typedef S<(0 < 0), int> S2_t, *S2_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef S<(0 < 0), int> S2_t, *S2_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using S2_t = S<(0 < 0), int>;
+// CHECK-FIXES-NEXT: using S2_p = S2_t*;
typedef S<(0 > 0 && (3 > 1) && (1 < 1)), int> S3_t;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -213,7 +221,9 @@ constexpr bool b[1] = {true};
typedef Q<b[0 < 0]> Q_t, *Q_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef Q<b[0 < 0]> Q_t, *Q_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Q_t = Q<b[0 < 0]>;
+// CHECK-FIXES-NEXT: using Q_p = Q_t*;
typedef Q<b[0 < 0]> Q2_t;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -227,7 +237,9 @@ struct T {
typedef Q<T{0 < 0}.b> Q3_t, *Q3_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef Q<T{0 < 0}.b> Q3_t, *Q3_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:27: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Q3_t = Q<T{0 < 0}.b>;
+// CHECK-FIXES-NEXT: using Q3_p = Q3_t*;
typedef Q<T{0 < 0}.b> Q3_t;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -246,4 +258,12 @@ typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0
typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > > Variadic_t, *Variadic_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > > Variadic_t, *Variadic_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:103: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Variadic_t = Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > >;
+// CHECK-FIXES-NEXT: using Variadic_p = Variadic_t*;
+
+typedef struct { int a; } R_t, *R_p;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using R_t = struct { int a; };
+// CHECK-FIXES-NEXT: using R_p = R_t*;
diff --git a/clang/include/clang/Basic/SourceLocation.h b/clang/include/clang/Basic/SourceLocation.h
index 4e569a1d0d1c..d3d18537dcc1 100644
--- a/clang/include/clang/Basic/SourceLocation.h
+++ b/clang/include/clang/Basic/SourceLocation.h
@@ -230,6 +230,11 @@ class SourceRange {
return B != X.B || E != X.E;
}
+ // Returns true iff other is wholly contained within this range.
+ bool fullyContains(const SourceRange &other) const {
+ return B <= other.B && E >= other.E;
+ }
+
void print(raw_ostream &OS, const SourceManager &SM) const;
std::string printToString(const SourceManager &SM) const;
void dump(const SourceManager &SM) const;
More information about the cfe-commits
mailing list