-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[clang-tidy] new check misc-use-internal-linkage #90830
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
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) ChangesAdd new check readability-mark-static to dectect variable and function can be marked as static. Full diff: https://github.com/llvm/llvm-project/pull/90830.diff 11 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 41065fc8e87859..3e0cbc4d943274 100644
--- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -23,6 +23,7 @@ add_clang_library(clangTidyReadabilityModule
IdentifierLengthCheck.cpp
IdentifierNamingCheck.cpp
ImplicitBoolConversionCheck.cpp
+ MarkStaticCheck.cpp
RedundantInlineSpecifierCheck.cpp
InconsistentDeclarationParameterNameCheck.cpp
IsolateDeclarationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/MarkStaticCheck.cpp b/clang-tools-extra/clang-tidy/readability/MarkStaticCheck.cpp
new file mode 100644
index 00000000000000..c8881963071de8
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/MarkStaticCheck.cpp
@@ -0,0 +1,65 @@
+//===--- MarkStaticCheck.cpp - clang-tidy ---------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MarkStaticCheck.h"
+#include "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Specifiers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::readability {
+
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isFirstDecl,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+ VarDecl)) {
+ return Node.isFirstDecl();
+}
+
+AST_MATCHER(Decl, isInMainFile) {
+ return Finder->getASTContext().getSourceManager().isInMainFile(
+ Node.getLocation());
+}
+
+AST_POLYMORPHIC_MATCHER(isExternStorageClass,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+ VarDecl)) {
+ return Node.getStorageClass() == SC_Extern;
+}
+
+} // namespace
+
+void MarkStaticCheck::registerMatchers(MatchFinder *Finder) {
+ auto Common =
+ allOf(isFirstDecl(), isInMainFile(),
+ unless(anyOf(isStaticStorageClass(), isExternStorageClass(),
+ isInAnonymousNamespace())));
+ Finder->addMatcher(
+ functionDecl(Common, unless(cxxMethodDecl()), unless(isMain()))
+ .bind("fn"),
+ this);
+ Finder->addMatcher(varDecl(Common, hasGlobalStorage()).bind("var"), this);
+}
+
+void MarkStaticCheck::check(const MatchFinder::MatchResult &Result) {
+ if (const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("fn")) {
+ diag(FD->getLocation(), "function %0 can be static") << FD;
+ return;
+ }
+ if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var")) {
+ diag(VD->getLocation(), "variable %0 can be static") << VD;
+ return;
+ }
+ llvm_unreachable("");
+}
+
+} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/MarkStaticCheck.h b/clang-tools-extra/clang-tidy/readability/MarkStaticCheck.h
new file mode 100644
index 00000000000000..0af6788d3e3d85
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/MarkStaticCheck.h
@@ -0,0 +1,33 @@
+//===--- MarkStaticCheck.h - clang-tidy -------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MARKSTATICCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MARKSTATICCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Detects variable and function can be marked as static.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/mark-static.html
+class MarkStaticCheck : public ClangTidyCheck {
+public:
+ MarkStaticCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MARKSTATICCHECK_H
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e5..5c04426818b5f7 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -32,6 +32,7 @@
#include "IsolateDeclarationCheck.h"
#include "MagicNumbersCheck.h"
#include "MakeMemberFunctionConstCheck.h"
+#include "MarkStaticCheck.h"
#include "MathMissingParenthesesCheck.h"
#include "MisleadingIndentationCheck.h"
#include "MisplacedArrayIndexCheck.h"
@@ -106,6 +107,7 @@ class ReadabilityModule : public ClangTidyModule {
"readability-identifier-naming");
CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
"readability-implicit-bool-conversion");
+ CheckFactories.registerCheck<MarkStaticCheck>("readability-mark-static");
CheckFactories.registerCheck<MathMissingParenthesesCheck>(
"readability-math-missing-parentheses");
CheckFactories.registerCheck<RedundantInlineSpecifierCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 5956ccb925485c..35e4d7e722f1de 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -151,6 +151,11 @@ New checks
Enforces consistent style for enumerators' initialization, covering three
styles: none, first only, or all initialized explicitly.
+- New :doc:`readability-mark-static
+ <clang-tidy/checks/readability/mark-static>` check.
+
+ Detects variable and function can be marked as static.
+
- New :doc:`readability-math-missing-parentheses
<clang-tidy/checks/readability/math-missing-parentheses>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 5cdaf9e35b6acd..4f473e13eb9480 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -364,6 +364,7 @@ Clang-Tidy Checks
:doc:`readability-isolate-declaration <readability/isolate-declaration>`, "Yes"
:doc:`readability-magic-numbers <readability/magic-numbers>`,
:doc:`readability-make-member-function-const <readability/make-member-function-const>`, "Yes"
+ :doc:`readability-mark-static <readability/mark-static>`,
:doc:`readability-math-missing-parentheses <readability/math-missing-parentheses>`, "Yes"
:doc:`readability-misleading-indentation <readability/misleading-indentation>`,
:doc:`readability-misplaced-array-index <readability/misplaced-array-index>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/mark-static.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/mark-static.rst
new file mode 100644
index 00000000000000..331e969ee061b9
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/mark-static.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - readability-mark-static
+
+readability-mark-static
+=======================
+
+Detects variable and function can be marked as static.
+
+Static functions and variables are scoped to a single file. Marking functions
+and variables as static helps to better remove dead code. In addition, it gives
+the compiler more information and can help compiler make more aggressive
+optimizations.
+
+Example:
+
+.. code-block:: c++
+ int v1; // can be marked as static
+
+ void fn1(); // can be marked as static
+
+ namespace {
+ // no need since already in anonymous namespace
+ int v2;
+ void fn2();
+ }
+ // no need since already in anonymous namespace
+ extern int v2;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
new file mode 100644
index 00000000000000..31c68e9ad0cd92
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
@@ -0,0 +1,3 @@
+#pragma once
+
+void func_header();
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
new file mode 100644
index 00000000000000..37e4cfbafff146
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
@@ -0,0 +1,3 @@
+#pragma once
+
+extern int gloabl_header;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/mark-static-func.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/mark-static-func.cpp
new file mode 100644
index 00000000000000..c9adcc70b8c5cb
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/mark-static-func.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s readability-mark-static %t -- -- -I%S/Inputs/mark-static-var
+
+#include "func.h"
+
+void func() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'func'
+
+template<class T>
+void func_template() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'func_template'
+
+struct S {
+ void method();
+};
+void S::method() {}
+
+void func_header();
+extern void func_extern();
+static void func_static();
+namespace {
+void func_anonymous_ns();
+} // namespace
+
+int main(int argc, const char*argv[]) {}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/mark-static-var.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/mark-static-var.cpp
new file mode 100644
index 00000000000000..f0b0486dd38c63
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/mark-static-var.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s readability-mark-static %t -- -- -I%S/Inputs/mark-static-var
+
+#include "var.h"
+
+int global;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'global'
+
+template<class T>
+T global_template;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: variable 'global_template'
+
+int gloabl_header;
+
+extern int global_extern;
+
+static int global_static;
+
+namespace {
+static int global_anonymous_ns;
+namespace NS {
+static int global_anonymous_ns;
+}
+}
+
+static void f(int para) {
+ int local;
+ static int local_static;
+}
+
+struct S {
+ int m1;
+ static int m2;
+};
+int S::m2;
|
7f42888
to
ec7ddff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Wrong check name, maybe readability-unessesary-external-linkage or something, maybe it should even be an performance check [as there it will bring more benefits] (current check suggest more an "static methods").
- Add support for unity builds (.cpp files included from single .cpp file = isInMainFile is not sufficient)
- Diagnostic need to be more detailed
- When I run into these issues, i got questions from developers why not use anonymous namespaces (maybe it could be configurable, or finall check restrict only to static [I prefer static])
Additionally when those issues were being fixed in project, there were lot of situations where there were forward declaration in header file, but developer forget to include header file in .cpp, this is why diagnostic need to be more detailed, so developer could find issue, instead of adding static.
In summary, my main concern is check name & diagnostic, current name is too generic
clang-tools-extra/docs/clang-tidy/checks/readability/mark-static.rst
Outdated
Show resolved
Hide resolved
For unity build, it looks like a project level decision. Then they can ignore this check. Maybe I can provide an option to treat some extension as main file also. |
Isn't this already covered by |
No, because -Wmissing-prototypes will not provide warning, when both declaration and definition is in .cpp |
Clang-tidy already got those options, check just need to utilize them. |
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/unnecessary-external-linkage.rst
Outdated
Show resolved
Hide resolved
Should I implement auto-fix for this check? Maybe some functions / variables will be marked incorrectly and cause link error because the coder just forget to include the header file but this check mark it as internal linkage. |
1bb5ed2
to
24cbbd0
Compare
clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
Outdated
Show resolved
Hide resolved
As a first step we can have it without auto-fix and add that as a second step once we figure out a good way to do it. |
329d6ea
to
f8fe43f
Compare
Co-authored-by: Danny Mösch <[email protected]>
…7/llvm-project into new-check-mark-static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding unit build:
google-global-names-in-headers
is one of the checks that check file extensions:
llvm-project/clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
Lines 42 to 44 in d33937b
if (!utils::isSpellingLocInHeaderFile( | |
D->getBeginLoc(), *Result.SourceManager, HeaderFileExtensions)) | |
return; |
Nice that there is a check for this now, it looks like there are a few instances of this issue in LLVM as well.
YES. I used such check on a project that I work for. In short it found over an 1000 issues, manually fixing them was not an option. Add an option for "static / anonymous namespace", and generate fixes/hints accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits.
Missing:
- more proper handling for unity files (.cpp included from .cpp)
- nits
- auto-fixes (any) - in worst case you can provide just fixes with static, but attaching them to notes.
return SM.isInMainFile(L) && | ||
!utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be:
#source files or main file if its not a header.
return utils::isSpellingLocInFile(L, SM, SourceFileExtensions) ||
(SM.isInMainFile(L) && !utils::isSpellingLocInFile(L, SM, HeaderFileExtensions));
isSpellingLocInHeaderFile should be renamed into isSpellingLocInFile, that "Header" is not needed in those functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will match more cases which I cannot understand the code style.
e.g.
/// a.cpp
void f(); // decl-1
/// b.cpp
#include <a.cpp>
void f() {} // decl-2
The current version will ignore this cases because decl-1 is not in main file.
But the suggestion version will catch this cases because both file is source fill extensions.
Actually I don't understand and didn't meet this code style, so I keep conservative to avoid false positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"because decl-1 is not in main file" but thats still source file and thats a point.
Take into account 2 use cases:
- user want to run check on a header files, in such case header file will be a main file, and check wont work.
- user want to run check on unity cpp, where there is single .cpp file generated by cmake that include other cpp files and those files include header files.
In both cases user should be able to do that.
You already got "allOf(isFirstDecl(), isInMainFile(HeaderFileExtensions)," and thats fine. so check should work only if first declaration is in source file (no additional header) or when declaration and definition is in header file, in such case adding static/anonymous namespace is also expected (but only if user explicitly want to run check on headers - by messing up with compile commands json for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the point. Our main disagreement is about
when declaration and definition is in header file, in such case adding static/anonymous namespace is also expected
In my opinion, we should not emit any wrong for the definition in header file. Add static in header file is dangerous and cause potential bug.
e.g.
/// a.hpp
PREFIX int f() {
static int a = 1;
return a++;
}
void b();
/// a.cpp
#include "a.hpp"
#include <iostream>
int main() {
std::cout << __func__ << " " << f() << "\n";
b();
std::cout << __func__ << " " << f() << "\n";
b();
}
/// b.cpp
#include "a.hpp"
#include <iostream>
void b() { std::cout << __func__ << " " << f() << "\n"; }
if PREFIX
is inline
, result is 1 2 3 4
.
if PREFIX
is static
, result is 1 2 1 2
.
if PREFIX
is inline static
, result is 1 2 1 2
.
if PREFIX
is template <int>
, result is 1 2 3 4
.
if PREFIX
is template <int> static
, result is 1 2 1 2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine for lack of fixes in header files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the expected behavior for unity files? use static or not? I am a little bit confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simply .cpp (source files) files included from main file should be treated like main file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a summary
If main file include non-header file (*.inc, *.cpp, etc..), this check should treat it as main file.
Then we just need to check there are no redecl
in header file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that FirstDecl stuff will work. just note that we got config for header extensions and source extensions. better would be to use source. as those .inc could be part of header files also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can recursive to find the real file of non-header file. Tracking the include chain and find the first file which includes this decl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I accept this change as it's fine for 1.0 version
Any fixed can be always done in this or new change.
return isInMainFile(D->getLocation(), | ||
Finder->getASTContext().getSourceManager(), | ||
HeaderFileExtensions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't needed.
All you need to do is just get location and:
return SM.isInMainFile(L) || !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
Or better add: isSpellingLocInSourceFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still I would just assume that if first declaration isn't in header file, then such declaration could be considered to be internal. As include for header should be there anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to tolerant some false negatives instead of introducing false positives for corner cases. It is more robust to check whether all redecls are in main file. And it will not cause performance issue since most of thing only have one declaration and one definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason of using recursive algorithm is there are some code practices will use macro and inc file to avoid duplicated code. In llvm-project, it is also used widely.
Add new check misc-use-internal-linkage to detect variable and function can be marked as static. --------- Co-authored-by: Danny Mösch <[email protected]> Signed-off-by: Hafidz Muzakky <[email protected]>
Add new check misc-use-internal-linkage to detect variable and function can be marked as static.