-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[clang-tidy] Make modernize-use-using
's fix-its more robust
#149694
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tidy Author: Victor Chernyakin (localspook) ChangesFixes #105503. First, let's look at typedef const int i;
// becomes
using i = int; // The const is gone!
typedef struct foo_s { int i; } * foo_t;
// becomes
using foo_t = struct foo_s { int i; }; // The pointer is gone!
typedef float vec3[3];
// Warns, but provides no fixit. With this PR, all Now let's look at // & and * are handled correctly.
typedef int i, &lval, *ptr;
// becomes
using i = int;
using lval = i&;
using ptr = i*;
// Anything else is not.
typedef int i, &&rval, vec3[3], *const const_ptr, (*fn)();
// becomes
using i = int;
using rval = int;
using const_ptr = i*;
using fn = int i, &&rval, vec3[3], *const const_ptr, (*)(); With this PR, all these cases are handled correctly. There is a preexisting issue that this PR does not fix though: llvm-project/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp Lines 388 to 392 in e7ac499
This test asserts incorrect behaviour: with the This PR does come with some regressions: Currently, we offer fixits when the int typedef i;
// becomes
using i = int; // ✅
int typedef * ptr;
// becomes
using ptr = int typedef *; // 💥 With this PR, we mishandle these cases. We also now mishandle cases where the introduced name is a macro: #define Macro foo
typedef int Macro; So, what do you think? I hope you'll agree that it's a net improvement: many common cases are handled better, and it's only weird cases that are handled worse. Patch is 22.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/149694.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
index 936a906651f16..fc49d7b820109 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -7,14 +7,12 @@
//===----------------------------------------------------------------------===//
#include "UseUsingCheck.h"
-#include "../utils/LexerUtils.h"
-#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TokenKinds.h"
#include "clang/Lex/Lexer.h"
-#include <string>
using namespace clang::ast_matchers;
namespace {
@@ -22,15 +20,13 @@ namespace {
AST_MATCHER(clang::LinkageSpecDecl, isExternCLinkage) {
return Node.getLanguage() == clang::LinkageSpecLanguageIDs::C;
}
+
} // namespace
namespace clang::tidy::modernize {
static constexpr llvm::StringLiteral ExternCDeclName = "extern-c-decl";
-static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
-static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
static constexpr llvm::StringLiteral TypedefName = "typedef";
-static constexpr llvm::StringLiteral DeclStmtName = "decl-stmt";
UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
@@ -47,66 +43,14 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
typedefDecl(
unless(isInstantiated()),
optionally(hasAncestor(
- linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))),
- anyOf(hasParent(decl().bind(ParentDeclName)),
- hasParent(declStmt().bind(DeclStmtName))))
+ linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))))
.bind(TypedefName),
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())),
- anyOf(hasParent(decl().bind(ParentDeclName)),
- hasParent(declStmt().bind(DeclStmtName)))),
- // We want the parent of the ClassTemplateDecl, not the parent
- // of the specialization.
- classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
- anyOf(hasParent(decl().bind(ParentDeclName)),
- hasParent(declStmt().bind(DeclStmtName))))))))
- .bind(TagDeclName),
- this);
}
void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *ParentDecl = Result.Nodes.getNodeAs<Decl>(ParentDeclName);
-
- if (!ParentDecl) {
- const auto *ParentDeclStmt = Result.Nodes.getNodeAs<DeclStmt>(DeclStmtName);
- if (ParentDeclStmt) {
- if (ParentDeclStmt->isSingleDecl())
- ParentDecl = ParentDeclStmt->getSingleDecl();
- else
- ParentDecl =
- ParentDeclStmt->getDeclGroup().getDeclGroup()
- [ParentDeclStmt->getDeclGroup().getDeclGroup().size() - 1];
- }
- }
-
- if (!ParentDecl)
- return;
-
- const SourceManager &SM = *Result.SourceManager;
- const LangOptions &LO = getLangOpts();
-
- // 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>(TagDeclName);
- if (MatchedTagDecl) {
- // 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.
- if (MatchedTagDecl->isThisDeclarationADefinition())
- LastTagDeclRanges[ParentDecl] = MatchedTagDecl->getSourceRange();
- return;
- }
-
- const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
- if (MatchedDecl->getLocation().isInvalid())
+ const auto &MatchedDecl = *Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
+ if (MatchedDecl.getLocation().isInvalid())
return;
const auto *ExternCDecl =
@@ -114,116 +58,50 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
if (ExternCDecl && IgnoreExternC)
return;
- SourceLocation StartLoc = MatchedDecl->getBeginLoc();
-
- if (StartLoc.isMacroID() && IgnoreMacros)
- return;
-
static constexpr llvm::StringLiteral UseUsingWarning =
"use 'using' instead of 'typedef'";
- // Warn at StartLoc but do not fix if there is macro or array.
- if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
- diag(StartLoc, UseUsingWarning);
+ if (MatchedDecl.getBeginLoc().isMacroID()) {
+ // Warn but do not fix if there is a macro.
+ if (!IgnoreMacros)
+ diag(MatchedDecl.getBeginLoc(), UseUsingWarning);
return;
}
- const TypeLoc TL = MatchedDecl->getTypeSourceInfo()->getTypeLoc();
-
- auto [Type, QualifierStr] = [MatchedDecl, this, &TL, &SM,
- &LO]() -> std::pair<std::string, std::string> {
- SourceRange TypeRange = TL.getSourceRange();
-
- // Function pointer case, get the left and right side of the identifier
- // without the identifier.
- if (TypeRange.fullyContains(MatchedDecl->getLocation())) {
- const auto RangeLeftOfIdentifier = CharSourceRange::getCharRange(
- TypeRange.getBegin(), MatchedDecl->getLocation());
- const auto RangeRightOfIdentifier = CharSourceRange::getCharRange(
- Lexer::getLocForEndOfToken(MatchedDecl->getLocation(), 0, SM, LO),
- Lexer::getLocForEndOfToken(TypeRange.getEnd(), 0, SM, LO));
- const std::string VerbatimType =
- (Lexer::getSourceText(RangeLeftOfIdentifier, SM, LO) +
- Lexer::getSourceText(RangeRightOfIdentifier, SM, LO))
- .str();
- return {VerbatimType, ""};
- }
-
- StringRef ExtraReference = "";
- if (MainTypeEndLoc.isValid() && TypeRange.fullyContains(MainTypeEndLoc)) {
- // Each type introduced in a typedef can specify being a reference or
- // pointer type seperately, so we need to sigure out if the new using-decl
- // needs to be to a reference or pointer as well.
- const SourceLocation Tok = utils::lexer::findPreviousAnyTokenKind(
- MatchedDecl->getLocation(), SM, LO, tok::TokenKind::star,
- tok::TokenKind::amp, tok::TokenKind::comma,
- tok::TokenKind::kw_typedef);
-
- ExtraReference = Lexer::getSourceText(
- CharSourceRange::getCharRange(Tok, Tok.getLocWithOffset(1)), SM, LO);
-
- if (ExtraReference != "*" && ExtraReference != "&")
- ExtraReference = "";
-
- TypeRange.setEnd(MainTypeEndLoc);
- }
- return {
- Lexer::getSourceText(CharSourceRange::getTokenRange(TypeRange), SM, LO)
- .str(),
- ExtraReference.str()};
- }();
- StringRef Name = MatchedDecl->getName();
- SourceRange ReplaceRange = MatchedDecl->getSourceRange();
+ const SourceManager &SM = *Result.SourceManager;
+ const LangOptions &LO = getLangOpts();
// 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.
- // But also we need to check that the ranges belong to the same file because
- // different files may contain overlapping ranges.
- std::string Using = "using ";
- if (ReplaceRange.getBegin().isMacroID() ||
- (Result.SourceManager->getFileID(ReplaceRange.getBegin()) !=
- Result.SourceManager->getFileID(LastReplacementEnd)) ||
- (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.str();
- MainTypeEndLoc = TL.getEndLoc();
+ const SourceRange RemovalRange = {
+ Lexer::findPreviousToken(MatchedDecl.getLocation(), SM, LO,
+ /*IncludeComments=*/true)
+ ->getEndLoc(),
+ Lexer::getLocForEndOfToken(MatchedDecl.getLocation(), 1, SM, LO)};
+ if (NextTypedefStartsANewSequence) {
+ diag(MatchedDecl.getBeginLoc(), UseUsingWarning)
+ << FixItHint::CreateReplacement(
+ {MatchedDecl.getBeginLoc(),
+ Lexer::getLocForEndOfToken(MatchedDecl.getBeginLoc(), 0, SM,
+ LO)},
+ ("using " + MatchedDecl.getName() + " =").str())
+ << FixItHint::CreateRemoval(RemovalRange);
+ FirstTypedefName = MatchedDecl.getName();
} 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 == FirstTypedefType && !QualifierStr.empty())
- Type = FirstTypedefName;
- }
-
- if (!ReplaceRange.getEnd().isMacroID()) {
- const SourceLocation::IntTy Offset =
- MatchedDecl->getFunctionType() ? 0 : Name.size();
- LastReplacementEnd = ReplaceRange.getEnd().getLocWithOffset(Offset);
+ diag(LastCommaOrSemi, UseUsingWarning)
+ << FixItHint::CreateReplacement(
+ LastCommaOrSemi,
+ (";\nusing " + MatchedDecl.getName() + " = " + FirstTypedefName)
+ .str())
+ << FixItHint::CreateRemoval(RemovalRange);
}
- auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
-
- // If typedef contains a full tag declaration, extract its full text.
- auto LastTagDeclRange = LastTagDeclRanges.find(ParentDecl);
- if (LastTagDeclRange != LastTagDeclRanges.end() &&
- LastTagDeclRange->second.isValid() &&
- ReplaceRange.fullyContains(LastTagDeclRange->second)) {
- Type = std::string(Lexer::getSourceText(
- CharSourceRange::getTokenRange(LastTagDeclRange->second), SM, LO));
- if (Type.empty())
- return;
- }
-
- std::string Replacement = (Using + Name + " = " + Type + QualifierStr).str();
- Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
+ const Token CommaOrSemi = *Lexer::findNextToken(
+ MatchedDecl.getEndLoc(), SM, LO, /*IncludeComments=*/false);
+ NextTypedefStartsANewSequence = CommaOrSemi.isNot(tok::TokenKind::comma);
+ LastCommaOrSemi = CommaOrSemi.getLocation();
}
+
} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
index 1e54bbf23c984..2f113e3d9e076 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -18,15 +18,11 @@ namespace clang::tidy::modernize {
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-using.html
class UseUsingCheck : public ClangTidyCheck {
-
const bool IgnoreMacros;
const bool IgnoreExternC;
- SourceLocation LastReplacementEnd;
- llvm::DenseMap<const Decl *, SourceRange> LastTagDeclRanges;
-
- std::string FirstTypedefType;
- std::string FirstTypedefName;
- SourceLocation MainTypeEndLoc;
+ bool NextTypedefStartsANewSequence = true;
+ StringRef FirstTypedefName;
+ SourceLocation LastCommaOrSemi;
public:
UseUsingCheck(StringRef Name, ClangTidyContext *Context);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 9eb3835fe8340..91899f88491b9 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -346,6 +346,10 @@ Changes in existing checks
whether function declarations and lambdas should be transformed by the check.
Fixed false positives when lambda was matched as a function in C++11 mode.
+- Improved :doc:`modernize-use-using
+ <clang-tidy/checks/modernize/use-using>` check by removing many incorrect
+ fixits and providing fixits where before there was only a warning.
+
- Improved :doc:`performance-move-const-arg
<clang-tidy/checks/performance/move-const-arg>` check by fixing false
negatives on ternary operators calling ``std::move``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp
index 092bc6666bb1c..c03cbc4d557fc 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp
@@ -9,11 +9,6 @@ CODE;
// CHECK-FIXES: CODE;
struct Foo;
-#define Bar Baz
-typedef Foo Bar;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: #define Bar Baz
-// CHECK-FIXES: using Baz = Foo;
#define TYPEDEF typedef
TYPEDEF Foo Bak;
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 8288f39126a11..b32a7b5722c52 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
@@ -1,9 +1,5 @@
// RUN: %check_clang_tidy %s modernize-use-using %t -- -- -fno-delayed-template-parsing -I %S/Inputs/use-using/
-typedef int Type;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
-// CHECK-FIXES: using Type = int;
-
typedef long LL;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using LL = long;
@@ -16,6 +12,18 @@ typedef Bla Bla2;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using Bla2 = Bla;
+typedef const int ConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using ConstInt = const int;
+
+typedef const int *const ConstPtrToConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using ConstPtrToConstInt = const int *const;
+
+typedef struct foo_s {} * foo_t;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using foo_t = struct foo_s {} *;
+
typedef void (*type)(int, int);
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using type = void (*)(int, int);
@@ -87,8 +95,24 @@ 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;
+// CHECK-FIXES-NEXT: using bla2 = bla1;
+// CHECK-FIXES-NEXT: using bla3 = bla1;
+
+typedef int I, &LVal, &&RVal, *Ptr, *const ConstPtr, Vec3[3], (*Fn)();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-3]]:21: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-4]]:29: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-5]]:35: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-6]]:52: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-7]]:61: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using I = int;
+// CHECK-FIXES-NEXT: using LVal = I &;
+// CHECK-FIXES-NEXT: using RVal = I &&;
+// CHECK-FIXES-NEXT: using Ptr = I *;
+// CHECK-FIXES-NEXT: using ConstPtr = I *const;
+// CHECK-FIXES-NEXT: using Vec3 = I[3];
+// CHECK-FIXES-NEXT: using Fn = I (*)();
#define CODE typedef int INT
@@ -97,11 +121,6 @@ CODE;
// CHECK-FIXES: CODE;
struct Foo;
-#define Bar Baz
-typedef Foo Bar;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: #define Bar Baz
-// CHECK-FIXES: using Baz = Foo;
#define TYPEDEF typedef
TYPEDEF Foo Bak;
@@ -118,38 +137,16 @@ typedef struct Foo Bap;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using Bap = struct Foo;
-struct Foo typedef Bap2;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Bap2 = struct Foo;
-
-Foo typedef Bap3;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Bap3 = Foo;
-
typedef struct Unknown Baq;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using Baq = struct Unknown;
-struct Unknown2 typedef Baw;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Baw = struct Unknown2;
-
-int typedef Bax;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Bax = int;
-
typedef struct Q1 { int a; } S1;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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: using S2 = struct { int b; };
-struct Q2 { int c; } typedef S3;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// 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: using S4 = struct { int d; };
namespace my_space {
class my_cclass {};
@@ -161,11 +158,7 @@ namespace my_space {
#define lol 4
typedef unsigned Map[lol];
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef unsigned Map[lol];
-
-typedef void (*fun_type)();
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using fun_type = void (*)();
+// CHECK-FIXES: using Map = unsigned[lol];
namespace template_instantiations {
template <typename T>
@@ -202,13 +195,13 @@ typedef S<(0 > 0), int> S_t, *S_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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*;
+// 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-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*;
+// 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'
@@ -223,7 +216,7 @@ typedef Q<b[0 < 0]> Q_t, *Q_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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*;
+// 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'
@@ -239,7 +232,7 @@ typedef Q<T{0 < 0}.b> Q3_t, *Q3_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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...
[truncated]
|
@llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) ChangesFixes #105503. First, let's look at typedef const int i;
// becomes
using i = int; // The const is gone!
typedef struct foo_s { int i; } * foo_t;
// becomes
using foo_t = struct foo_s { int i; }; // The pointer is gone!
typedef float vec3[3];
// Warns, but provides no fixit. With this PR, all Now let's look at // & and * are handled correctly.
typedef int i, &lval, *ptr;
// becomes
using i = int;
using lval = i&;
using ptr = i*;
// Anything else is not.
typedef int i, &&rval, vec3[3], *const const_ptr, (*fn)();
// becomes
using i = int;
using rval = int;
using const_ptr = i*;
using fn = int i, &&rval, vec3[3], *const const_ptr, (*)(); With this PR, all these cases are handled correctly. There is a preexisting issue that this PR does not fix though: llvm-project/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp Lines 388 to 392 in e7ac499
This test asserts incorrect behaviour: with the This PR does come with some regressions: Currently, we offer fixits when the int typedef i;
// becomes
using i = int; // ✅
int typedef * ptr;
// becomes
using ptr = int typedef *; // 💥 With this PR, we mishandle these cases. We also now mishandle cases where the introduced name is a macro: #define Macro foo
typedef int Macro; So, what do you think? I hope you'll agree that it's a net improvement: many common cases are handled better, and it's only weird cases that are handled worse. Patch is 22.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/149694.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
index 936a906651f16..fc49d7b820109 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -7,14 +7,12 @@
//===----------------------------------------------------------------------===//
#include "UseUsingCheck.h"
-#include "../utils/LexerUtils.h"
-#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TokenKinds.h"
#include "clang/Lex/Lexer.h"
-#include <string>
using namespace clang::ast_matchers;
namespace {
@@ -22,15 +20,13 @@ namespace {
AST_MATCHER(clang::LinkageSpecDecl, isExternCLinkage) {
return Node.getLanguage() == clang::LinkageSpecLanguageIDs::C;
}
+
} // namespace
namespace clang::tidy::modernize {
static constexpr llvm::StringLiteral ExternCDeclName = "extern-c-decl";
-static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
-static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
static constexpr llvm::StringLiteral TypedefName = "typedef";
-static constexpr llvm::StringLiteral DeclStmtName = "decl-stmt";
UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
@@ -47,66 +43,14 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
typedefDecl(
unless(isInstantiated()),
optionally(hasAncestor(
- linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))),
- anyOf(hasParent(decl().bind(ParentDeclName)),
- hasParent(declStmt().bind(DeclStmtName))))
+ linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))))
.bind(TypedefName),
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())),
- anyOf(hasParent(decl().bind(ParentDeclName)),
- hasParent(declStmt().bind(DeclStmtName)))),
- // We want the parent of the ClassTemplateDecl, not the parent
- // of the specialization.
- classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
- anyOf(hasParent(decl().bind(ParentDeclName)),
- hasParent(declStmt().bind(DeclStmtName))))))))
- .bind(TagDeclName),
- this);
}
void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *ParentDecl = Result.Nodes.getNodeAs<Decl>(ParentDeclName);
-
- if (!ParentDecl) {
- const auto *ParentDeclStmt = Result.Nodes.getNodeAs<DeclStmt>(DeclStmtName);
- if (ParentDeclStmt) {
- if (ParentDeclStmt->isSingleDecl())
- ParentDecl = ParentDeclStmt->getSingleDecl();
- else
- ParentDecl =
- ParentDeclStmt->getDeclGroup().getDeclGroup()
- [ParentDeclStmt->getDeclGroup().getDeclGroup().size() - 1];
- }
- }
-
- if (!ParentDecl)
- return;
-
- const SourceManager &SM = *Result.SourceManager;
- const LangOptions &LO = getLangOpts();
-
- // 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>(TagDeclName);
- if (MatchedTagDecl) {
- // 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.
- if (MatchedTagDecl->isThisDeclarationADefinition())
- LastTagDeclRanges[ParentDecl] = MatchedTagDecl->getSourceRange();
- return;
- }
-
- const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
- if (MatchedDecl->getLocation().isInvalid())
+ const auto &MatchedDecl = *Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
+ if (MatchedDecl.getLocation().isInvalid())
return;
const auto *ExternCDecl =
@@ -114,116 +58,50 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
if (ExternCDecl && IgnoreExternC)
return;
- SourceLocation StartLoc = MatchedDecl->getBeginLoc();
-
- if (StartLoc.isMacroID() && IgnoreMacros)
- return;
-
static constexpr llvm::StringLiteral UseUsingWarning =
"use 'using' instead of 'typedef'";
- // Warn at StartLoc but do not fix if there is macro or array.
- if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
- diag(StartLoc, UseUsingWarning);
+ if (MatchedDecl.getBeginLoc().isMacroID()) {
+ // Warn but do not fix if there is a macro.
+ if (!IgnoreMacros)
+ diag(MatchedDecl.getBeginLoc(), UseUsingWarning);
return;
}
- const TypeLoc TL = MatchedDecl->getTypeSourceInfo()->getTypeLoc();
-
- auto [Type, QualifierStr] = [MatchedDecl, this, &TL, &SM,
- &LO]() -> std::pair<std::string, std::string> {
- SourceRange TypeRange = TL.getSourceRange();
-
- // Function pointer case, get the left and right side of the identifier
- // without the identifier.
- if (TypeRange.fullyContains(MatchedDecl->getLocation())) {
- const auto RangeLeftOfIdentifier = CharSourceRange::getCharRange(
- TypeRange.getBegin(), MatchedDecl->getLocation());
- const auto RangeRightOfIdentifier = CharSourceRange::getCharRange(
- Lexer::getLocForEndOfToken(MatchedDecl->getLocation(), 0, SM, LO),
- Lexer::getLocForEndOfToken(TypeRange.getEnd(), 0, SM, LO));
- const std::string VerbatimType =
- (Lexer::getSourceText(RangeLeftOfIdentifier, SM, LO) +
- Lexer::getSourceText(RangeRightOfIdentifier, SM, LO))
- .str();
- return {VerbatimType, ""};
- }
-
- StringRef ExtraReference = "";
- if (MainTypeEndLoc.isValid() && TypeRange.fullyContains(MainTypeEndLoc)) {
- // Each type introduced in a typedef can specify being a reference or
- // pointer type seperately, so we need to sigure out if the new using-decl
- // needs to be to a reference or pointer as well.
- const SourceLocation Tok = utils::lexer::findPreviousAnyTokenKind(
- MatchedDecl->getLocation(), SM, LO, tok::TokenKind::star,
- tok::TokenKind::amp, tok::TokenKind::comma,
- tok::TokenKind::kw_typedef);
-
- ExtraReference = Lexer::getSourceText(
- CharSourceRange::getCharRange(Tok, Tok.getLocWithOffset(1)), SM, LO);
-
- if (ExtraReference != "*" && ExtraReference != "&")
- ExtraReference = "";
-
- TypeRange.setEnd(MainTypeEndLoc);
- }
- return {
- Lexer::getSourceText(CharSourceRange::getTokenRange(TypeRange), SM, LO)
- .str(),
- ExtraReference.str()};
- }();
- StringRef Name = MatchedDecl->getName();
- SourceRange ReplaceRange = MatchedDecl->getSourceRange();
+ const SourceManager &SM = *Result.SourceManager;
+ const LangOptions &LO = getLangOpts();
// 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.
- // But also we need to check that the ranges belong to the same file because
- // different files may contain overlapping ranges.
- std::string Using = "using ";
- if (ReplaceRange.getBegin().isMacroID() ||
- (Result.SourceManager->getFileID(ReplaceRange.getBegin()) !=
- Result.SourceManager->getFileID(LastReplacementEnd)) ||
- (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.str();
- MainTypeEndLoc = TL.getEndLoc();
+ const SourceRange RemovalRange = {
+ Lexer::findPreviousToken(MatchedDecl.getLocation(), SM, LO,
+ /*IncludeComments=*/true)
+ ->getEndLoc(),
+ Lexer::getLocForEndOfToken(MatchedDecl.getLocation(), 1, SM, LO)};
+ if (NextTypedefStartsANewSequence) {
+ diag(MatchedDecl.getBeginLoc(), UseUsingWarning)
+ << FixItHint::CreateReplacement(
+ {MatchedDecl.getBeginLoc(),
+ Lexer::getLocForEndOfToken(MatchedDecl.getBeginLoc(), 0, SM,
+ LO)},
+ ("using " + MatchedDecl.getName() + " =").str())
+ << FixItHint::CreateRemoval(RemovalRange);
+ FirstTypedefName = MatchedDecl.getName();
} 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 == FirstTypedefType && !QualifierStr.empty())
- Type = FirstTypedefName;
- }
-
- if (!ReplaceRange.getEnd().isMacroID()) {
- const SourceLocation::IntTy Offset =
- MatchedDecl->getFunctionType() ? 0 : Name.size();
- LastReplacementEnd = ReplaceRange.getEnd().getLocWithOffset(Offset);
+ diag(LastCommaOrSemi, UseUsingWarning)
+ << FixItHint::CreateReplacement(
+ LastCommaOrSemi,
+ (";\nusing " + MatchedDecl.getName() + " = " + FirstTypedefName)
+ .str())
+ << FixItHint::CreateRemoval(RemovalRange);
}
- auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
-
- // If typedef contains a full tag declaration, extract its full text.
- auto LastTagDeclRange = LastTagDeclRanges.find(ParentDecl);
- if (LastTagDeclRange != LastTagDeclRanges.end() &&
- LastTagDeclRange->second.isValid() &&
- ReplaceRange.fullyContains(LastTagDeclRange->second)) {
- Type = std::string(Lexer::getSourceText(
- CharSourceRange::getTokenRange(LastTagDeclRange->second), SM, LO));
- if (Type.empty())
- return;
- }
-
- std::string Replacement = (Using + Name + " = " + Type + QualifierStr).str();
- Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
+ const Token CommaOrSemi = *Lexer::findNextToken(
+ MatchedDecl.getEndLoc(), SM, LO, /*IncludeComments=*/false);
+ NextTypedefStartsANewSequence = CommaOrSemi.isNot(tok::TokenKind::comma);
+ LastCommaOrSemi = CommaOrSemi.getLocation();
}
+
} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
index 1e54bbf23c984..2f113e3d9e076 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -18,15 +18,11 @@ namespace clang::tidy::modernize {
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-using.html
class UseUsingCheck : public ClangTidyCheck {
-
const bool IgnoreMacros;
const bool IgnoreExternC;
- SourceLocation LastReplacementEnd;
- llvm::DenseMap<const Decl *, SourceRange> LastTagDeclRanges;
-
- std::string FirstTypedefType;
- std::string FirstTypedefName;
- SourceLocation MainTypeEndLoc;
+ bool NextTypedefStartsANewSequence = true;
+ StringRef FirstTypedefName;
+ SourceLocation LastCommaOrSemi;
public:
UseUsingCheck(StringRef Name, ClangTidyContext *Context);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 9eb3835fe8340..91899f88491b9 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -346,6 +346,10 @@ Changes in existing checks
whether function declarations and lambdas should be transformed by the check.
Fixed false positives when lambda was matched as a function in C++11 mode.
+- Improved :doc:`modernize-use-using
+ <clang-tidy/checks/modernize/use-using>` check by removing many incorrect
+ fixits and providing fixits where before there was only a warning.
+
- Improved :doc:`performance-move-const-arg
<clang-tidy/checks/performance/move-const-arg>` check by fixing false
negatives on ternary operators calling ``std::move``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp
index 092bc6666bb1c..c03cbc4d557fc 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using-macros.cpp
@@ -9,11 +9,6 @@ CODE;
// CHECK-FIXES: CODE;
struct Foo;
-#define Bar Baz
-typedef Foo Bar;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: #define Bar Baz
-// CHECK-FIXES: using Baz = Foo;
#define TYPEDEF typedef
TYPEDEF Foo Bak;
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 8288f39126a11..b32a7b5722c52 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
@@ -1,9 +1,5 @@
// RUN: %check_clang_tidy %s modernize-use-using %t -- -- -fno-delayed-template-parsing -I %S/Inputs/use-using/
-typedef int Type;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
-// CHECK-FIXES: using Type = int;
-
typedef long LL;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using LL = long;
@@ -16,6 +12,18 @@ typedef Bla Bla2;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using Bla2 = Bla;
+typedef const int ConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using ConstInt = const int;
+
+typedef const int *const ConstPtrToConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using ConstPtrToConstInt = const int *const;
+
+typedef struct foo_s {} * foo_t;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using foo_t = struct foo_s {} *;
+
typedef void (*type)(int, int);
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using type = void (*)(int, int);
@@ -87,8 +95,24 @@ 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;
+// CHECK-FIXES-NEXT: using bla2 = bla1;
+// CHECK-FIXES-NEXT: using bla3 = bla1;
+
+typedef int I, &LVal, &&RVal, *Ptr, *const ConstPtr, Vec3[3], (*Fn)();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-3]]:21: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-4]]:29: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-5]]:35: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-6]]:52: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-7]]:61: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using I = int;
+// CHECK-FIXES-NEXT: using LVal = I &;
+// CHECK-FIXES-NEXT: using RVal = I &&;
+// CHECK-FIXES-NEXT: using Ptr = I *;
+// CHECK-FIXES-NEXT: using ConstPtr = I *const;
+// CHECK-FIXES-NEXT: using Vec3 = I[3];
+// CHECK-FIXES-NEXT: using Fn = I (*)();
#define CODE typedef int INT
@@ -97,11 +121,6 @@ CODE;
// CHECK-FIXES: CODE;
struct Foo;
-#define Bar Baz
-typedef Foo Bar;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: #define Bar Baz
-// CHECK-FIXES: using Baz = Foo;
#define TYPEDEF typedef
TYPEDEF Foo Bak;
@@ -118,38 +137,16 @@ typedef struct Foo Bap;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using Bap = struct Foo;
-struct Foo typedef Bap2;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Bap2 = struct Foo;
-
-Foo typedef Bap3;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Bap3 = Foo;
-
typedef struct Unknown Baq;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using Baq = struct Unknown;
-struct Unknown2 typedef Baw;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Baw = struct Unknown2;
-
-int typedef Bax;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Bax = int;
-
typedef struct Q1 { int a; } S1;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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: using S2 = struct { int b; };
-struct Q2 { int c; } typedef S3;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// 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: using S4 = struct { int d; };
namespace my_space {
class my_cclass {};
@@ -161,11 +158,7 @@ namespace my_space {
#define lol 4
typedef unsigned Map[lol];
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef unsigned Map[lol];
-
-typedef void (*fun_type)();
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using fun_type = void (*)();
+// CHECK-FIXES: using Map = unsigned[lol];
namespace template_instantiations {
template <typename T>
@@ -202,13 +195,13 @@ typedef S<(0 > 0), int> S_t, *S_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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*;
+// 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-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*;
+// 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'
@@ -223,7 +216,7 @@ typedef Q<b[0 < 0]> Q_t, *Q_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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*;
+// 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'
@@ -239,7 +232,7 @@ typedef Q<T{0 < 0}.b> Q3_t, *Q3_p;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// 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...
[truncated]
|
5836727
to
823bfd4
Compare
modernize-use-using
's fixits more robustmodernize-use-using
's fix-its more robust
Fixes #105503.
Fixes #31141 (the new fix-it is not the one the author suggests, but now it's at least correct).
First, let's look at
typedef
s that only introduce one name. Currently, this fails in several cases:With this PR, all
typedef
s that only introduce one name are handled correctly, with a fix-it.Now let's look at
typedef
s that introduce multiple names. Here's the current situation:With this PR, all these cases are handled correctly.
There is a preexisting issue that this PR does not fix though:
llvm-project/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp
Lines 388 to 392 in e7ac499
This test asserts incorrect behaviour: with the
typedef
, bothint_ptr
andint_ptr_ptr
areint *
; the nameint_ptr_ptr
is wrong. The fix-it transforms them intoint *
andint **
respectively. The fundamental problem is that we have no way to understand that, givenint* int_ptr
, theint
applies to all the introduced names, while the*
only applies to the first name.This PR does come with some regressions:
Currently, we offer fix-its when the
typedef
is not the first thing in thedecl-specifier-seq
. This sometimes works, sometimes doesn't:With this PR, we mishandle these cases. We also now mishandle cases where the introduced name is a macro:
So, what do you think? I hope you'll agree that it's a net improvement: many common cases are handled better, and it's only weird cases that are handled worse.