-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
Merged
Merged
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
24cbbd0
reformat
HerrCai0907 fb55ca3
rename to misc-use-internal-linkage
HerrCai0907 f8fe43f
fix
HerrCai0907 91a798f
Update clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
HerrCai0907 3b3dda3
fix doc
HerrCai0907 d940f5b
Merge branch 'new-check-mark-static' of https://github.com/HerrCai090…
HerrCai0907 eda2fad
fix comments
HerrCai0907 a67b602
fix
HerrCai0907 65e11fa
Merge remote-tracking branch 'upstream/main' into new-check-mark-static
HerrCai0907 11544d6
Merge branch 'main' into new-check-mark-static
HerrCai0907 82c274d
Merge branch 'main' of https://github.com/llvm/llvm-project into new-…
HerrCai0907 25f7d64
Recursive search for include files in non-header files
HerrCai0907 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
78 changes: 78 additions & 0 deletions
78
clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
//===--- UseInternalLinkageCheck.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 "UseInternalLinkageCheck.h" | ||
#include "../utils/FileExtensionsUtils.h" | ||
#include "clang/AST/Decl.h" | ||
#include "clang/ASTMatchers/ASTMatchFinder.h" | ||
#include "clang/ASTMatchers/ASTMatchers.h" | ||
#include "clang/ASTMatchers/ASTMatchersMacros.h" | ||
#include "clang/Basic/SourceLocation.h" | ||
#include "clang/Basic/Specifiers.h" | ||
#include "llvm/ADT/STLExtras.h" | ||
|
||
using namespace clang::ast_matchers; | ||
|
||
namespace clang::tidy::misc { | ||
|
||
namespace { | ||
|
||
AST_MATCHER(Decl, isFirstDecl) { return Node.isFirstDecl(); } | ||
|
||
AST_MATCHER_P(Decl, isInMainFile, FileExtensionsSet, HeaderFileExtensions) { | ||
return llvm::all_of(Node.redecls(), [&](const Decl *D) { | ||
SourceManager &SM = Finder->getASTContext().getSourceManager(); | ||
const SourceLocation L = D->getLocation(); | ||
return SM.isInMainFile(L) && | ||
!utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions); | ||
}); | ||
} | ||
|
||
AST_POLYMORPHIC_MATCHER(isExternStorageClass, | ||
AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl, | ||
VarDecl)) { | ||
return Node.getStorageClass() == SC_Extern; | ||
} | ||
|
||
} // namespace | ||
|
||
void UseInternalLinkageCheck::registerMatchers(MatchFinder *Finder) { | ||
auto Common = allOf(isFirstDecl(), isInMainFile(HeaderFileExtensions), | ||
PiotrZSL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
unless(anyOf( | ||
// 1. internal linkage | ||
isStaticStorageClass(), isInAnonymousNamespace(), | ||
// 2. explicit external linkage | ||
isExternStorageClass(), isExternC(), | ||
// 3. template | ||
isExplicitTemplateSpecialization(), | ||
// 4. friend | ||
hasAncestor(friendDecl())))); | ||
Finder->addMatcher( | ||
functionDecl(Common, unless(cxxMethodDecl()), unless(isMain())) | ||
.bind("fn"), | ||
this); | ||
Finder->addMatcher(varDecl(Common, hasGlobalStorage()).bind("var"), this); | ||
} | ||
|
||
static constexpr StringRef Message = | ||
"%0 %1 can be made static or moved into an anonymous namespace " | ||
"to enforce internal linkage"; | ||
|
||
void UseInternalLinkageCheck::check(const MatchFinder::MatchResult &Result) { | ||
if (const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("fn")) { | ||
diag(FD->getLocation(), Message) << "function" << FD; | ||
return; | ||
} | ||
if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var")) { | ||
diag(VD->getLocation(), Message) << "variable" << VD; | ||
return; | ||
} | ||
5chmidti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
llvm_unreachable(""); | ||
} | ||
|
||
} // namespace clang::tidy::misc |
38 changes: 38 additions & 0 deletions
38
clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
//===--- UseInternalLinkageCheck.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_MISC_USEINTERNALLINKAGECHECK_H | ||
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEINTERNALLINKAGECHECK_H | ||
|
||
#include "../ClangTidyCheck.h" | ||
|
||
namespace clang::tidy::misc { | ||
|
||
/// Detects variables and functions that can be marked as static or moved into | ||
/// an anonymous namespace to enforce internal linkage. | ||
/// | ||
/// For the user-facing documentation see: | ||
/// http://clang.llvm.org/extra/clang-tidy/checks/misc/use-internal-linkage.html | ||
class UseInternalLinkageCheck : public ClangTidyCheck { | ||
public: | ||
UseInternalLinkageCheck(StringRef Name, ClangTidyContext *Context) | ||
: ClangTidyCheck(Name, Context), | ||
HeaderFileExtensions(Context->getHeaderFileExtensions()) {} | ||
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; | ||
} | ||
|
||
private: | ||
FileExtensionsSet HeaderFileExtensions; | ||
}; | ||
|
||
} // namespace clang::tidy::misc | ||
|
||
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEINTERNALLINKAGECHECK_H |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
27 changes: 27 additions & 0 deletions
27
clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
.. title:: clang-tidy - misc-use-internal-linkage | ||
|
||
misc-use-internal-linkage | ||
========================= | ||
|
||
Detects variables and functions that can be marked as static or moved into | ||
an anonymous namespace to enforce internal linkage. | ||
|
||
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 allows for more aggressive optimizations. | ||
|
||
Example: | ||
|
||
.. code-block:: c++ | ||
|
||
int v1; // can be marked as static | ||
|
||
void fn1(); // can be marked as static | ||
|
||
namespace { | ||
// already in anonymous namespace | ||
int v2; | ||
void fn2(); | ||
} | ||
// already declared as extern | ||
extern int v2; |
3 changes: 3 additions & 0 deletions
3
clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/use-internal-linkage/func.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
#pragma once | ||
|
||
void func_header(); |
3 changes: 3 additions & 0 deletions
3
clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/use-internal-linkage/var.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
#pragma once | ||
|
||
extern int gloabl_header; |
30 changes: 30 additions & 0 deletions
30
clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-func.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// RUN: %check_clang_tidy %s misc-use-internal-linkage %t -- -- -I%S/Inputs/use-internal-linkage | ||
|
||
#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[]) {} | ||
|
||
extern "C" { | ||
void func_extern_c_1() {} | ||
} | ||
|
||
extern "C" void func_extern_c_2() {} |
40 changes: 40 additions & 0 deletions
40
clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-var.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// RUN: %check_clang_tidy %s misc-use-internal-linkage %t -- -- -I%S/Inputs/use-internal-linkage | ||
|
||
#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; | ||
|
||
extern "C" { | ||
int global_in_extern_c_1; | ||
} | ||
|
||
extern "C" int global_in_extern_c_2; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.
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:
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
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.
if
PREFIX
isinline
, result is1 2 3 4
.if
PREFIX
isstatic
, result is1 2 1 2
.if
PREFIX
isinline static
, result is1 2 1 2
.if
PREFIX
istemplate <int>
, result is1 2 3 4
.if
PREFIX
istemplate <int> static
, result is1 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.