Skip to content

Commit 5836727

Browse files
localspooklocalspook
committed
[clang-tidy] Make modernize-use-using's fixits more robust
1 parent 1c541aa commit 5836727

File tree

5 files changed

+82
-223
lines changed

5 files changed

+82
-223
lines changed

‎clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp

Lines changed: 36 additions & 158 deletions
Original file line numberDiff line numberDiff line change
@@ -7,30 +7,26 @@
77
//===----------------------------------------------------------------------===//
88

99
#include "UseUsingCheck.h"
10-
#include "../utils/LexerUtils.h"
11-
#include "clang/AST/DeclGroup.h"
10+
#include "clang/Basic/Diagnostic.h"
1211
#include "clang/Basic/LangOptions.h"
1312
#include "clang/Basic/SourceLocation.h"
1413
#include "clang/Basic/SourceManager.h"
1514
#include "clang/Basic/TokenKinds.h"
1615
#include "clang/Lex/Lexer.h"
17-
#include <string>
1816

1917
using namespace clang::ast_matchers;
2018
namespace {
2119

2220
AST_MATCHER(clang::LinkageSpecDecl, isExternCLinkage) {
2321
return Node.getLanguage() == clang::LinkageSpecLanguageIDs::C;
2422
}
23+
2524
} // namespace
2625

2726
namespace clang::tidy::modernize {
2827

2928
static constexpr llvm::StringLiteral ExternCDeclName = "extern-c-decl";
30-
static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
31-
static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
3229
static constexpr llvm::StringLiteral TypedefName = "typedef";
33-
static constexpr llvm::StringLiteral DeclStmtName = "decl-stmt";
3430

3531
UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
3632
: ClangTidyCheck(Name, Context),
@@ -47,183 +43,65 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
4743
typedefDecl(
4844
unless(isInstantiated()),
4945
optionally(hasAncestor(
50-
linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))),
51-
anyOf(hasParent(decl().bind(ParentDeclName)),
52-
hasParent(declStmt().bind(DeclStmtName))))
46+
linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))))
5347
.bind(TypedefName),
5448
this);
55-
56-
// This matcher is used to find tag declarations in source code within
57-
// typedefs. They appear in the AST just *prior* to the typedefs.
58-
Finder->addMatcher(
59-
tagDecl(
60-
anyOf(allOf(unless(anyOf(isImplicit(),
61-
classTemplateSpecializationDecl())),
62-
anyOf(hasParent(decl().bind(ParentDeclName)),
63-
hasParent(declStmt().bind(DeclStmtName)))),
64-
// We want the parent of the ClassTemplateDecl, not the parent
65-
// of the specialization.
66-
classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
67-
anyOf(hasParent(decl().bind(ParentDeclName)),
68-
hasParent(declStmt().bind(DeclStmtName))))))))
69-
.bind(TagDeclName),
70-
this);
7149
}
7250

7351
void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
74-
const auto *ParentDecl = Result.Nodes.getNodeAs<Decl>(ParentDeclName);
75-
76-
if (!ParentDecl) {
77-
const auto *ParentDeclStmt = Result.Nodes.getNodeAs<DeclStmt>(DeclStmtName);
78-
if (ParentDeclStmt) {
79-
if (ParentDeclStmt->isSingleDecl())
80-
ParentDecl = ParentDeclStmt->getSingleDecl();
81-
else
82-
ParentDecl =
83-
ParentDeclStmt->getDeclGroup().getDeclGroup()
84-
[ParentDeclStmt->getDeclGroup().getDeclGroup().size() - 1];
85-
}
86-
}
87-
88-
if (!ParentDecl)
89-
return;
90-
91-
const SourceManager &SM = *Result.SourceManager;
92-
const LangOptions &LO = getLangOpts();
93-
94-
// Match CXXRecordDecl only to store the range of the last non-implicit full
95-
// declaration, to later check whether it's within the typdef itself.
96-
const auto *MatchedTagDecl = Result.Nodes.getNodeAs<TagDecl>(TagDeclName);
97-
if (MatchedTagDecl) {
98-
// It is not sufficient to just track the last TagDecl that we've seen,
99-
// because if one struct or union is nested inside another, the last TagDecl
100-
// before the typedef will be the nested one (PR#50990). Therefore, we also
101-
// keep track of the parent declaration, so that we can look up the last
102-
// TagDecl that is a sibling of the typedef in the AST.
103-
if (MatchedTagDecl->isThisDeclarationADefinition())
104-
LastTagDeclRanges[ParentDecl] = MatchedTagDecl->getSourceRange();
105-
return;
106-
}
107-
108-
const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
109-
if (MatchedDecl->getLocation().isInvalid())
52+
const auto &MatchedDecl = *Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
53+
if (MatchedDecl.getLocation().isInvalid())
11054
return;
11155

11256
const auto *ExternCDecl =
11357
Result.Nodes.getNodeAs<LinkageSpecDecl>(ExternCDeclName);
11458
if (ExternCDecl && IgnoreExternC)
11559
return;
11660

117-
SourceLocation StartLoc = MatchedDecl->getBeginLoc();
118-
119-
if (StartLoc.isMacroID() && IgnoreMacros)
120-
return;
121-
12261
static constexpr llvm::StringLiteral UseUsingWarning =
12362
"use 'using' instead of 'typedef'";
12463

125-
// Warn at StartLoc but do not fix if there is macro or array.
126-
if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
127-
diag(StartLoc, UseUsingWarning);
64+
if (MatchedDecl.getBeginLoc().isMacroID()) {
65+
// Warn but do not fix if there is a macro.
66+
if (!IgnoreMacros)
67+
diag(MatchedDecl.getBeginLoc(), UseUsingWarning);
12868
return;
12969
}
13070

131-
const TypeLoc TL = MatchedDecl->getTypeSourceInfo()->getTypeLoc();
132-
133-
auto [Type, QualifierStr] = [MatchedDecl, this, &TL, &SM,
134-
&LO]() -> std::pair<std::string, std::string> {
135-
SourceRange TypeRange = TL.getSourceRange();
136-
137-
// Function pointer case, get the left and right side of the identifier
138-
// without the identifier.
139-
if (TypeRange.fullyContains(MatchedDecl->getLocation())) {
140-
const auto RangeLeftOfIdentifier = CharSourceRange::getCharRange(
141-
TypeRange.getBegin(), MatchedDecl->getLocation());
142-
const auto RangeRightOfIdentifier = CharSourceRange::getCharRange(
143-
Lexer::getLocForEndOfToken(MatchedDecl->getLocation(), 0, SM, LO),
144-
Lexer::getLocForEndOfToken(TypeRange.getEnd(), 0, SM, LO));
145-
const std::string VerbatimType =
146-
(Lexer::getSourceText(RangeLeftOfIdentifier, SM, LO) +
147-
Lexer::getSourceText(RangeRightOfIdentifier, SM, LO))
148-
.str();
149-
return {VerbatimType, ""};
150-
}
151-
152-
StringRef ExtraReference = "";
153-
if (MainTypeEndLoc.isValid() && TypeRange.fullyContains(MainTypeEndLoc)) {
154-
// Each type introduced in a typedef can specify being a reference or
155-
// pointer type seperately, so we need to sigure out if the new using-decl
156-
// needs to be to a reference or pointer as well.
157-
const SourceLocation Tok = utils::lexer::findPreviousAnyTokenKind(
158-
MatchedDecl->getLocation(), SM, LO, tok::TokenKind::star,
159-
tok::TokenKind::amp, tok::TokenKind::comma,
160-
tok::TokenKind::kw_typedef);
161-
162-
ExtraReference = Lexer::getSourceText(
163-
CharSourceRange::getCharRange(Tok, Tok.getLocWithOffset(1)), SM, LO);
164-
165-
if (ExtraReference != "*" && ExtraReference != "&")
166-
ExtraReference = "";
167-
168-
TypeRange.setEnd(MainTypeEndLoc);
169-
}
170-
return {
171-
Lexer::getSourceText(CharSourceRange::getTokenRange(TypeRange), SM, LO)
172-
.str(),
173-
ExtraReference.str()};
174-
}();
175-
StringRef Name = MatchedDecl->getName();
176-
SourceRange ReplaceRange = MatchedDecl->getSourceRange();
71+
const SourceManager &SM = *Result.SourceManager;
72+
const LangOptions &LO = getLangOpts();
17773

17874
// typedefs with multiple comma-separated definitions produce multiple
17975
// consecutive TypedefDecl nodes whose SourceRanges overlap. Each range starts
18076
// at the "typedef" and then continues *across* previous definitions through
18177
// the end of the current TypedefDecl definition.
182-
// But also we need to check that the ranges belong to the same file because
183-
// different files may contain overlapping ranges.
184-
std::string Using = "using ";
185-
if (ReplaceRange.getBegin().isMacroID() ||
186-
(Result.SourceManager->getFileID(ReplaceRange.getBegin()) !=
187-
Result.SourceManager->getFileID(LastReplacementEnd)) ||
188-
(ReplaceRange.getBegin() >= LastReplacementEnd)) {
189-
// This is the first (and possibly the only) TypedefDecl in a typedef. Save
190-
// Type and Name in case we find subsequent TypedefDecl's in this typedef.
191-
FirstTypedefType = Type;
192-
FirstTypedefName = Name.str();
193-
MainTypeEndLoc = TL.getEndLoc();
78+
const SourceRange RemovalRange = {
79+
Lexer::findPreviousToken(MatchedDecl.getLocation(), SM, LO,
80+
/*IncludeComments=*/true)
81+
->getEndLoc(),
82+
Lexer::getLocForEndOfToken(MatchedDecl.getLocation(), 1, SM, LO)};
83+
if (NextTypedefStartsANewSequence) {
84+
diag(MatchedDecl.getBeginLoc(), UseUsingWarning)
85+
<< FixItHint::CreateReplacement(
86+
{MatchedDecl.getBeginLoc(),
87+
Lexer::getLocForEndOfToken(MatchedDecl.getBeginLoc(), 0, SM,
88+
LO)},
89+
("using " + MatchedDecl.getName() + " =").str())
90+
<< FixItHint::CreateRemoval(RemovalRange);
91+
FirstTypedefName = MatchedDecl.getName();
19492
} else {
195-
// This is additional TypedefDecl in a comma-separated typedef declaration.
196-
// Start replacement *after* prior replacement and separate with semicolon.
197-
ReplaceRange.setBegin(LastReplacementEnd);
198-
Using = ";\nusing ";
199-
200-
// If this additional TypedefDecl's Type starts with the first TypedefDecl's
201-
// type, make this using statement refer back to the first type, e.g. make
202-
// "typedef int Foo, *Foo_p;" -> "using Foo = int;\nusing Foo_p = Foo*;"
203-
if (Type == FirstTypedefType && !QualifierStr.empty())
204-
Type = FirstTypedefName;
205-
}
206-
207-
if (!ReplaceRange.getEnd().isMacroID()) {
208-
const SourceLocation::IntTy Offset =
209-
MatchedDecl->getFunctionType() ? 0 : Name.size();
210-
LastReplacementEnd = ReplaceRange.getEnd().getLocWithOffset(Offset);
93+
diag(LastCommaOrSemi, UseUsingWarning)
94+
<< FixItHint::CreateReplacement(
95+
LastCommaOrSemi,
96+
(";\nusing " + MatchedDecl.getName() + " = " + FirstTypedefName)
97+
.str())
98+
<< FixItHint::CreateRemoval(RemovalRange);
21199
}
212100

213-
auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
214-
215-
// If typedef contains a full tag declaration, extract its full text.
216-
auto LastTagDeclRange = LastTagDeclRanges.find(ParentDecl);
217-
if (LastTagDeclRange != LastTagDeclRanges.end() &&
218-
LastTagDeclRange->second.isValid() &&
219-
ReplaceRange.fullyContains(LastTagDeclRange->second)) {
220-
Type = std::string(Lexer::getSourceText(
221-
CharSourceRange::getTokenRange(LastTagDeclRange->second), SM, LO));
222-
if (Type.empty())
223-
return;
224-
}
225-
226-
std::string Replacement = (Using + Name + " = " + Type + QualifierStr).str();
227-
Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
101+
const Token CommaOrSemi = *Lexer::findNextToken(
102+
MatchedDecl.getEndLoc(), SM, LO, /*IncludeComments=*/false);
103+
NextTypedefStartsANewSequence = CommaOrSemi.isNot(tok::TokenKind::comma);
104+
LastCommaOrSemi = CommaOrSemi.getLocation();
228105
}
106+
229107
} // namespace clang::tidy::modernize

‎clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,11 @@ namespace clang::tidy::modernize {
1818
/// For the user-facing documentation see:
1919
/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-using.html
2020
class UseUsingCheck : public ClangTidyCheck {
21-
2221
const bool IgnoreMacros;
2322
const bool IgnoreExternC;
24-
SourceLocation LastReplacementEnd;
25-
llvm::DenseMap<const Decl *, SourceRange> LastTagDeclRanges;
26-
27-
std::string FirstTypedefType;
28-
std::string FirstTypedefName;
29-
SourceLocation MainTypeEndLoc;
23+
bool NextTypedefStartsANewSequence = true;
24+
StringRef FirstTypedefName;
25+
SourceLocation LastCommaOrSemi;
3026

3127
public:
3228
UseUsingCheck(StringRef Name, ClangTidyContext *Context);

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,10 @@ Changes in existing checks
346346
whether function declarations and lambdas should be transformed by the check.
347347
Fixed false positives when lambda was matched as a function in C++11 mode.
348348

349+
- Improved :doc:`modernize-use-using
350+
<clang-tidy/checks/modernize/use-using>` check by removing many incorrect
351+
fixits and providing fixits where before there was only a warning.
352+
349353
- Improved :doc:`performance-move-const-arg
350354
<clang-tidy/checks/performance/move-const-arg>` check by fixing false
351355
negatives on ternary operators calling ``std::move``.

‎clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,6 @@ CODE;
99
// CHECK-FIXES: CODE;
1010

1111
struct Foo;
12-
#define Bar Baz
13-
typedef Foo Bar;
14-
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
15-
// CHECK-FIXES: #define Bar Baz
16-
// CHECK-FIXES: using Baz = Foo;
1712

1813
#define TYPEDEF typedef
1914
TYPEDEF Foo Bak;

0 commit comments

Comments
 (0)