Skip to content

[clang-tidy] Fix support for typedefs in readability-identifier-naming #66835

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

Conversation

PiotrZSL
Copy link
Member

@PiotrZSL PiotrZSL commented Sep 19, 2023

Typedef rename were not properly handled when typedef were used behind pointer, or as a part of function type. Additionally because entire function were passed as an source-range, when function started with macro, such change were not marked for a fix.

Removed workaround and used proper TypedefTypeLoc instead.

Fixes #55156, #54699

Typedef rename were not properly handled when typedef were used
behind pointer, or as a part of function type. Additionally
because entire function were passed as an source-range, when
function started with macro, such change were not marked for a fix.

Removed workaround and used proper TypedefTypeLoc instead.
@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2023

@llvm/pr-subscribers-clang-tidy

Changes

Typedef rename were not properly handled when typedef were used behind pointer, or as a part of function type. Additionally because entire function were passed as an source-range, when function started with macro, such change were not marked for a fix.

Removed workaround and used proper TypedefTypeLoc instead.

Fixes #55156


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

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp (+5-20)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3-1)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp (+18)
diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
index d24b7a65b1c4334..da1433aa2d05d47 100644
--- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
@@ -256,26 +256,6 @@ class RenamerClangTidyVisitor
       return true;
     }
 
-    // Fix type aliases in value declarations.
-    if (const auto *Value = dyn_cast<ValueDecl>(Decl)) {
-      if (const Type *TypePtr = Value->getType().getTypePtrOrNull()) {
-        if (const auto *Typedef = TypePtr->getAs<TypedefType>())
-          Check->addUsage(Typedef->getDecl(), Value->getSourceRange(), SM);
-      }
-    }
-
-    // Fix type aliases in function declarations.
-    if (const auto *Value = dyn_cast<FunctionDecl>(Decl)) {
-      if (const auto *Typedef =
-              Value->getReturnType().getTypePtr()->getAs<TypedefType>())
-        Check->addUsage(Typedef->getDecl(), Value->getSourceRange(), SM);
-      for (const ParmVarDecl *Param : Value->parameters()) {
-        if (const TypedefType *Typedef =
-                Param->getType().getTypePtr()->getAs<TypedefType>())
-          Check->addUsage(Typedef->getDecl(), Value->getSourceRange(), SM);
-      }
-    }
-
     // Fix overridden methods
     if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
       if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) {
@@ -340,6 +320,11 @@ class RenamerClangTidyVisitor
     return true;
   }
 
+  bool VisitTypedefTypeLoc(const TypedefTypeLoc &Loc) {
+    Check->addUsage(Loc.getTypedefNameDecl(), Loc.getSourceRange(), SM);
+    return true;
+  }
+
   bool VisitTagTypeLoc(const TagTypeLoc &Loc) {
     Check->addUsage(Loc.getDecl(), Loc.getSourceRange(), SM);
     return true;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6d6f51998a01e57..b8977a39239ffce 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -291,7 +291,9 @@ Changes in existing checks
   warnings when a type's forward declaration precedes its definition.
   Additionally, it now provides appropriate warnings for ``struct`` and
   ``union`` in C, while also incorporating support for the
-  ``Leading_upper_snake_case`` naming convention.
+  ``Leading_upper_snake_case`` naming convention. The handling of ``typedef``
+  has been enhanced, particularly within complex types like function pointers
+  and cases where style checks were omitted when functions started with macros.
 
 - Improved :doc:`readability-implicit-bool-conversion
   <clang-tidy/checks/readability/implicit-bool-conversion>` check to take
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp
index 3445a21bfdbc4aa..0f5fc1145f8791e 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp
@@ -729,3 +729,21 @@ struct forward_declared_as_struct;
 class forward_declared_as_struct {
 };
 
+namespace pr55156 {
+
+typedef enum {
+  VALUE0,
+  VALUE1,
+} ValueType;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: invalid case style for typedef 'ValueType' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}} value_type_t;
+
+#define STATIC_MACRO static
+STATIC_MACRO void someFunc(ValueType a_v1, const ValueType& a_v2) {}
+// CHECK-FIXES: {{^}}STATIC_MACRO void someFunc(value_type_t a_v1, const value_type_t& a_v2) {}
+STATIC_MACRO void someFunc(const ValueType** p_a_v1, ValueType (*p_a_v2)()) {}
+// CHECK-FIXES: {{^}}STATIC_MACRO void someFunc(const value_type_t** p_a_v1, value_type_t (*p_a_v2)()) {}
+STATIC_MACRO ValueType someFunc() {}
+// CHECK-FIXES: {{^}}STATIC_MACRO value_type_t someFunc() {}
+#undef STATIC_MACRO
+}

Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@carlosgalvezp carlosgalvezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix and the great cleanup!

@PiotrZSL PiotrZSL merged commit 5d95d27 into llvm:main Sep 20, 2023
@PiotrZSL PiotrZSL deleted the 55156-clang-tidy-fails-to-detect-readability-identifier-namingtypedefprefix-check-in-case-of-macro-usage branch September 20, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants