[clang-tidy] use specified type verbatim in modernize-use-using fix #113837

5chmidti · 2024-10-27T20:54:06Z

Previously, the implementation used the printed type, which contains expanded
macro arguments, deletes comments, and removes function argument names
from the alias declaration. Instead, this check can be more surgical and use the
actual written type verbatim.

Fixes #33760
Fixes #37846
Fixes #41685
Fixes #83568
Fixes #95716
Fixes #97009

Previously, the implementation used the printed type, which contains the macro arguments expanded (also deleting comments). Instead, this check can be more surgical and keep the actual written type as is, keeping macros unexpanded. Fixes llvm#33760, llvm#37846

llvmbot · 2024-10-27T20:54:39Z

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Julian Schmidt (5chmidti)

Changes

Previously, the implementation used the printed type, which contains the
macro arguments expanded (also deleting comments). Instead, this
check can be more surgical and keep the actual written type as is,
keeping macros unexpanded.

Fixes #33760
Fixes #37846

Full diff: https://github.com/llvm/llvm-project/pull/113837.diff

4 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp (+58-13)
(modified) clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h (+1)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+3)
(modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp (+31-5)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
index 24eefdb082eb32..74764d95d90520 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -7,9 +7,12 @@
 //===----------------------------------------------------------------------===//
 
 #include "UseUsingCheck.h"
-#include "clang/AST/ASTContext.h"
+#include "../utils/LexerUtils.h"
 #include "clang/AST/DeclGroup.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/Lexer.h"
+#include <string>
 
 using namespace clang::ast_matchers;
 namespace {
@@ -119,14 +122,55 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
     return;
   }
 
-  PrintingPolicy PrintPolicy(getLangOpts());
-  PrintPolicy.SuppressScope = true;
-  PrintPolicy.ConstantArraySizeAsWritten = true;
-  PrintPolicy.UseVoidForZeroParams = false;
-  PrintPolicy.PrintInjectedClassNameWithArguments = false;
+  const TypeLoc TL = MatchedDecl->getTypeSourceInfo()->getTypeLoc();
+
+  auto [Type, QualifierStr] = [MatchedDecl, &Result, this,
+                               &TL]() -> 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())) {
+      return {(Lexer::getSourceText(
+                   CharSourceRange::getCharRange(TL.getBeginLoc(),
+                                                 MatchedDecl->getLocation()),
+                   *Result.SourceManager, getLangOpts()) +
+               Lexer::getSourceText(
+                   CharSourceRange::getCharRange(
+                       Lexer::getLocForEndOfToken(MatchedDecl->getLocation(), 0,
+                                                  *Result.SourceManager,
+                                                  getLangOpts()),
+                       Lexer::getLocForEndOfToken(TL.getEndLoc(), 0,
+                                                  *Result.SourceManager,
+                                                  getLangOpts())),
+                   *Result.SourceManager, getLangOpts()))
+                  .str(),
+              ""};
+    }
+
+    StringRef ExtraReference = "";
+    if (MainTypeEndLoc.isValid() && TypeRange.fullyContains(MainTypeEndLoc)) {
+      const SourceLocation Tok = utils::lexer::findPreviousAnyTokenKind(
+          MatchedDecl->getLocation(), *Result.SourceManager, getLangOpts(),
+          tok::TokenKind::star, tok::TokenKind::amp, tok::TokenKind::comma,
+          tok::TokenKind::kw_typedef);
+
+      ExtraReference = Lexer::getSourceText(
+          CharSourceRange::getCharRange(Tok, Tok.getLocWithOffset(1)),
+          *Result.SourceManager, getLangOpts());
 
-  std::string Type = MatchedDecl->getUnderlyingType().getAsString(PrintPolicy);
-  std::string Name = MatchedDecl->getNameAsString();
+      if (ExtraReference != "*" && ExtraReference != "&")
+        ExtraReference = "";
+
+      if (MainTypeEndLoc.isValid())
+        TypeRange.setEnd(MainTypeEndLoc);
+    }
+    return {Lexer::getSourceText(CharSourceRange::getTokenRange(TypeRange),
+                                 *Result.SourceManager, getLangOpts())
+                .str(),
+            ExtraReference.str()};
+  }();
+  StringRef Name = MatchedDecl->getName();
   SourceRange ReplaceRange = MatchedDecl->getSourceRange();
 
   // typedefs with multiple comma-separated definitions produce multiple
@@ -143,7 +187,8 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
     // 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;
+    FirstTypedefName = Name.str();
+    MainTypeEndLoc = TL.getEndLoc();
   } else {
     // This is additional TypedefDecl in a comma-separated typedef declaration.
     // Start replacement *after* prior replacement and separate with semicolon.
@@ -153,10 +198,10 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
     // 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 (Type == FirstTypedefType && !QualifierStr.empty())
+      Type = FirstTypedefName;
   }
+
   if (!ReplaceRange.getEnd().isMacroID()) {
     const SourceLocation::IntTy Offset =
         MatchedDecl->getFunctionType() ? 0 : Name.size();
@@ -177,7 +222,7 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
       return;
   }
 
-  std::string Replacement = Using + Name + " = " + Type;
+  std::string Replacement = (Using + Name + " = " + Type + QualifierStr).str();
   Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
 }
 } // 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 7054778d84a0c6..1e54bbf23c984f 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -26,6 +26,7 @@ class UseUsingCheck : public ClangTidyCheck {
 
   std::string FirstTypedefType;
   std::string FirstTypedefName;
+  SourceLocation MainTypeEndLoc;
 
 public:
   UseUsingCheck(StringRef Name, ClangTidyContext *Context);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 876689c40fcdb2..fe98aa9b9fd4b8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -228,6 +228,9 @@ Changes in existing checks
   member function calls too and to only expand macros starting with ``PRI``
   and ``__PRI`` from ``<inttypes.h>`` in the format string.
 
+- Improved :doc:`modernize-use-using
+  <clang-tidy/checks/modernize/use-using>` check by not expanding macros.
+
 - Improved :doc:`performance-avoid-endl
   <clang-tidy/checks/performance/avoid-endl>` check to use ``std::endl`` as
   placeholder when lexer cannot get source text.
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 925e5f9c1ca54e..7833078f171bf8 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
@@ -80,7 +80,7 @@ typedef Test<my_class *> another;
 
 typedef int* PInt;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using PInt = int *;
+// CHECK-FIXES: using PInt = int*;
 
 typedef int bla1, bla2, bla3;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -112,7 +112,7 @@ TYPEDEF Foo Bak;
 typedef FOO Bam;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: #define FOO Foo
-// CHECK-FIXES: using Bam = Foo;
+// CHECK-FIXES: using Bam = FOO;
 
 typedef struct Foo Bap;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -247,7 +247,7 @@ typedef Q<T{0 < 0}.b> Q3_t;
 
 typedef TwoArgTemplate<TwoArgTemplate<int, Q<T{0 < 0}.b> >, S<(0 < 0), Q<b[0 < 0]> > > Nested_t;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using Nested_t = TwoArgTemplate<TwoArgTemplate<int, Q<T{0 < 0}.b>>, S<(0 < 0), Q<b[0 < 0]>>>;
+// CHECK-FIXES: using Nested_t = TwoArgTemplate<TwoArgTemplate<int, Q<T{0 < 0}.b> >, S<(0 < 0), Q<b[0 < 0]> > >;
 
 template <typename a>
 class TemplateKeyword {
@@ -265,12 +265,12 @@ class Variadic {};
 
 typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > > Variadic_t;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: 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: using Variadic_t = Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 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-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: 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;
@@ -383,3 +383,29 @@ namespace ISSUE_72179
   // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: use 'using' instead of 'typedef' [modernize-use-using]
   // CHECK-FIXES: const auto foo4 = [](int a){using d = int;};
 }
+
+
+typedef int* int_ptr, *int_ptr_ptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-MESSAGES: :[[@LINE-2]]:21: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using int_ptr = int*;
+// CHECK-FIXES-NEXT: using int_ptr_ptr = int_ptr*;
+
+#ifndef SpecialMode
+#define SomeMacro(x) x
+#else
+#define SomeMacro(x) SpecialType
+#endif
+
+class SomeMacro(GH33760) { };
+
+typedef void(SomeMacro(GH33760)::* FunctionType)(float, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using FunctionType = void(SomeMacro(GH33760)::* )(float, int);
+
+#define CDECL __attribute((cdecl))
+
+typedef void (CDECL *GH37846)(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using GH37846 = void (CDECL *)(int);
+

5chmidti · 2024-11-25T20:16:41Z

Ping

HerrCai0907 · 2024-11-27T14:57:34Z

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

+    if (TypeRange.fullyContains(MatchedDecl->getLocation())) {
+      return {(Lexer::getSourceText(
+                   CharSourceRange::getCharRange(TL.getBeginLoc(),
+                                                 MatchedDecl->getLocation()),
+                   *Result.SourceManager, getLangOpts()) +
+               Lexer::getSourceText(
+                   CharSourceRange::getCharRange(
+                       Lexer::getLocForEndOfToken(MatchedDecl->getLocation(), 0,
+                                                  *Result.SourceManager,
+                                                  getLangOpts()),
+                       Lexer::getLocForEndOfToken(TL.getEndLoc(), 0,
+                                                  *Result.SourceManager,
+                                                  getLangOpts())),
+                   *Result.SourceManager, getLangOpts()))
+                  .str(),
+              ""};
+    }


Could you somehow refactor this part of code. It is too hard to understand what is the meaning of each substring due to indentation

Is something like this what you had in mind?

5chmidti

I noticed some more issues that this is fixing, so I've added their examples as test-cases

5chmidti · 2024-12-01T23:16:54Z

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

+    if (TypeRange.fullyContains(MatchedDecl->getLocation())) {
+      return {(Lexer::getSourceText(
+                   CharSourceRange::getCharRange(TL.getBeginLoc(),
+                                                 MatchedDecl->getLocation()),
+                   *Result.SourceManager, getLangOpts()) +
+               Lexer::getSourceText(
+                   CharSourceRange::getCharRange(
+                       Lexer::getLocForEndOfToken(MatchedDecl->getLocation(), 0,
+                                                  *Result.SourceManager,
+                                                  getLangOpts()),
+                       Lexer::getLocForEndOfToken(TL.getEndLoc(), 0,
+                                                  *Result.SourceManager,
+                                                  getLangOpts())),
+                   *Result.SourceManager, getLangOpts()))
+                  .str(),
+              ""};
+    }


Is something like this what you had in mind?

5chmidti · 2024-12-15T22:14:51Z

Ping

HerrCai0907

LGTM

llvmbot added clang-tools-extra clang-tidy labels Oct 27, 2024

EugeneZelenko requested review from PiotrZSL, SimplyDanny and HerrCai0907 October 28, 2024 14:40

5chmidti mentioned this pull request Nov 25, 2024

modernize-use-using only works for trivial typedefs #38677

Closed

HerrCai0907 reviewed Nov 27, 2024

View reviewed changes

5chmidti added 5 commits December 1, 2024 02:37

add tests for additionally fixed issues

14ff78e

address review comment regarding readability of getting the source text

91d4465

remove already checked condition

d722beb

create SM local var

ded9e5e

create LO var for lang opts

cf95ee2

5chmidti changed the title ~~[clang-tidy] do not expand macros in modernize-use-using fix~~ [clang-tidy] use specified type verbatim in modernize-use-using fix Dec 1, 2024

5chmidti commented Dec 1, 2024

View reviewed changes

5chmidti requested a review from HerrCai0907 December 15, 2024 22:15

HerrCai0907 approved these changes Dec 16, 2024

View reviewed changes

5chmidti merged commit f77152d into llvm:main Dec 21, 2024
9 checks passed

5chmidti deleted the clang_tidy_use_using_fixit branch December 21, 2024 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-tidy] use specified type verbatim in modernize-use-using fix #113837

[clang-tidy] use specified type verbatim in modernize-use-using fix #113837

Uh oh!

5chmidti commented Oct 27, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 27, 2024 •

edited

Loading

Uh oh!

5chmidti commented Nov 25, 2024

Uh oh!

HerrCai0907 Nov 27, 2024

Uh oh!

5chmidti Dec 1, 2024

Uh oh!

5chmidti left a comment

Uh oh!

5chmidti Dec 1, 2024

Uh oh!

5chmidti commented Dec 15, 2024

Uh oh!

HerrCai0907 left a comment

Uh oh!

Uh oh!

Uh oh!

[clang-tidy] use specified type verbatim in modernize-use-using fix #113837

[clang-tidy] use specified type verbatim in modernize-use-using fix #113837

Uh oh!

Conversation

5chmidti commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

5chmidti commented Nov 25, 2024

Uh oh!

HerrCai0907 Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

5chmidti Dec 1, 2024

Choose a reason for hiding this comment

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

5chmidti Dec 1, 2024

Choose a reason for hiding this comment

Uh oh!

5chmidti commented Dec 15, 2024

Uh oh!

HerrCai0907 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

5chmidti commented Oct 27, 2024 •

edited

Loading

llvmbot commented Oct 27, 2024 •

edited

Loading