Skip to content

[clang-tidy] fix insertion location for function pointers in cppcoreguidelines-init-variables #112091

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

Merged

Conversation

5chmidti
Copy link
Contributor

Previously, the insertion location for the = nullptr fix would be
after the variable name. However, if the variable is of type function
pointer that is not an alias, then the insertion would happen inside the
type specification: void (*a1)(void*); -> void (*a1 = nullptr)(void*);.
With this change, the insertion location will be at the next
'terminator'. That is, at the next , or ;, as that will finish the
current declaration: void (a1)(void*) = nullptr;.

Fixes #112089

…uidelines-init-variables

Previously, the insertion location for the `= nullptr` fix would be
after the variable name. However, if the variable is of type function
pointer that is not an alias, then the insertion would happen inside the
type specification: `void (*a1)(void*);` -> `void (*a1 = nullptr)(void*);`.
With this change, the insertion location will be at the next
'terminator'. That is, at the next `,` or `;`, as that will finish the
current declaration: `void (a1)(void*) = nullptr;`.

Fixes llvm#112089
@llvmbot
Copy link
Member

llvmbot commented Oct 12, 2024

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

Author: Julian Schmidt (5chmidti)

Changes

Previously, the insertion location for the = nullptr fix would be
after the variable name. However, if the variable is of type function
pointer that is not an alias, then the insertion would happen inside the
type specification: void (*a1)(void*); -> void (*a1 = nullptr)(void*);.
With this change, the insertion location will be at the next
'terminator'. That is, at the next , or ;, as that will finish the
current declaration: void (a1)(void*) = nullptr;.

Fixes #112089


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

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp (+5-3)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+7-3)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp (+14)
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
index bdba2314c7056f..3eef2fd12cc8e5 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -8,9 +8,10 @@
 
 #include "InitVariablesCheck.h"
 
+#include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include <optional>
 
@@ -107,8 +108,9 @@ void InitVariablesCheck::check(const MatchFinder::MatchResult &Result) {
         << MatchedDecl;
     if (*InitializationString != nullptr)
       Diagnostic << FixItHint::CreateInsertion(
-          MatchedDecl->getLocation().getLocWithOffset(
-              MatchedDecl->getName().size()),
+          utils::lexer::findNextTerminator(MatchedDecl->getLocation(),
+                                           *Result.SourceManager,
+                                           Result.Context->getLangOpts()),
           *InitializationString);
     if (AddMathInclude) {
       Diagnostic << IncludeInserter.createIncludeInsertion(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3f7bcde1eb3014..7836ebb8a9e57d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -171,6 +171,10 @@ Changes in existing checks
   fix false positive that floating point variable is only used in increment
   expression.
 
+- Improved :doc:`cppcoreguidelines-init-variables
+  <clang-tidy/checks/cppcoreguidelines-init-variables>` check by fixing the
+  insertion location for function pointers.
+
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
   avoid false positive when member initialization depends on a structured
@@ -185,9 +189,9 @@ Changes in existing checks
   false positive for C++23 deducing this.
 
 - Improved :doc:`modernize-avoid-c-arrays
-  <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest using ``std::span``
-  as a replacement for parameters of incomplete C array type in C++20 and 
-  ``std::array`` or ``std::vector`` before C++20.
+  <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest using 
+  ``std::span`` as a replacement for parameters of incomplete C array type in
+  C++20 and ``std::array`` or ``std::vector`` before C++20.
 
 - Improved :doc:`modernize-loop-convert
   <clang-tidy/checks/modernize/loop-convert>` check to fix false positive when
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
index e3d50946d1cb8f..824431c1bf52fd 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -134,3 +134,17 @@ void test_clang_diagnostic_error() {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' [clang-diagnostic-error]
   // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
 }
+
+namespace gh112089 {
+    void foo(void*);
+    using FPtr = void(*)(void*);
+    void test() {
+        void(*a1)(void*);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'a1' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: void(*a1)(void*) = nullptr;
+        FPtr a2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: variable 'a2' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: FPtr a2 = nullptr;
+    }
+} // namespace gh112089
+

@llvmbot
Copy link
Member

llvmbot commented Oct 12, 2024

@llvm/pr-subscribers-clang-tidy

Author: Julian Schmidt (5chmidti)

Changes

Previously, the insertion location for the = nullptr fix would be
after the variable name. However, if the variable is of type function
pointer that is not an alias, then the insertion would happen inside the
type specification: void (*a1)(void*); -> void (*a1 = nullptr)(void*);.
With this change, the insertion location will be at the next
'terminator'. That is, at the next , or ;, as that will finish the
current declaration: void (a1)(void*) = nullptr;.

Fixes #112089


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

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp (+5-3)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+7-3)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp (+14)
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
index bdba2314c7056f..3eef2fd12cc8e5 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -8,9 +8,10 @@
 
 #include "InitVariablesCheck.h"
 
+#include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include <optional>
 
@@ -107,8 +108,9 @@ void InitVariablesCheck::check(const MatchFinder::MatchResult &Result) {
         << MatchedDecl;
     if (*InitializationString != nullptr)
       Diagnostic << FixItHint::CreateInsertion(
-          MatchedDecl->getLocation().getLocWithOffset(
-              MatchedDecl->getName().size()),
+          utils::lexer::findNextTerminator(MatchedDecl->getLocation(),
+                                           *Result.SourceManager,
+                                           Result.Context->getLangOpts()),
           *InitializationString);
     if (AddMathInclude) {
       Diagnostic << IncludeInserter.createIncludeInsertion(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3f7bcde1eb3014..7836ebb8a9e57d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -171,6 +171,10 @@ Changes in existing checks
   fix false positive that floating point variable is only used in increment
   expression.
 
+- Improved :doc:`cppcoreguidelines-init-variables
+  <clang-tidy/checks/cppcoreguidelines-init-variables>` check by fixing the
+  insertion location for function pointers.
+
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
   avoid false positive when member initialization depends on a structured
@@ -185,9 +189,9 @@ Changes in existing checks
   false positive for C++23 deducing this.
 
 - Improved :doc:`modernize-avoid-c-arrays
-  <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest using ``std::span``
-  as a replacement for parameters of incomplete C array type in C++20 and 
-  ``std::array`` or ``std::vector`` before C++20.
+  <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest using 
+  ``std::span`` as a replacement for parameters of incomplete C array type in
+  C++20 and ``std::array`` or ``std::vector`` before C++20.
 
 - Improved :doc:`modernize-loop-convert
   <clang-tidy/checks/modernize/loop-convert>` check to fix false positive when
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
index e3d50946d1cb8f..824431c1bf52fd 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp
@@ -134,3 +134,17 @@ void test_clang_diagnostic_error() {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' [clang-diagnostic-error]
   // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
 }
+
+namespace gh112089 {
+    void foo(void*);
+    using FPtr = void(*)(void*);
+    void test() {
+        void(*a1)(void*);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'a1' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: void(*a1)(void*) = nullptr;
+        FPtr a2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: variable 'a2' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: FPtr a2 = nullptr;
+    }
+} // namespace gh112089
+

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

LGTM

@5chmidti 5chmidti merged commit 028ea71 into llvm:main Nov 10, 2024
9 checks passed
@5chmidti 5chmidti deleted the clang_tidy_init_variables_function_ptr_location branch November 10, 2024 17:26
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
…uidelines-init-variables (llvm#112091)

Previously, the insertion location for the `= nullptr` fix would be
after the variable name. However, if the variable is of type function
pointer that is not an alias, then the insertion would happen inside the
type specification: `void (*a1)(void*);` -> `void (*a1 =
nullptr)(void*);`.
With this change, the insertion location will be at the next
'terminator'. That is, at the next `,` or `;`, as that will finish the
current declaration: `void (a1)(void*) = nullptr;`.

Fixes llvm#112089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy] when try to use code action to fix the function pointer not initialized problem, clang-tidy add =nullptr to the wrong place
3 participants