Skip to content

[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.

Sign up for GitHub

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 12 commits into from
Jun 7, 2024
Merged

[clang-tidy] new check misc-use-internal-linkage #90830

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
24cbbd0
reformat
HerrCai0907 May 2, 2024
fb55ca3
rename to misc-use-internal-linkage
HerrCai0907 May 4, 2024
f8fe43f
fix
HerrCai0907 May 4, 2024
91a798f
Update clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
HerrCai0907 May 4, 2024
3b3dda3
fix doc
HerrCai0907 May 5, 2024
d940f5b
Merge branch 'new-check-mark-static' of https://github.com/HerrCai090…
HerrCai0907 May 5, 2024
eda2fad
fix comments
HerrCai0907 May 6, 2024
a67b602
fix
HerrCai0907 May 6, 2024
65e11fa
Merge remote-tracking branch 'upstream/main' into new-check-mark-static
HerrCai0907 May 14, 2024
11544d6
Merge branch 'main' into new-check-mark-static
HerrCai0907 May 26, 2024
82c274d
Merge branch 'main' of https://github.com/llvm/llvm-project into new-…
HerrCai0907 Jun 6, 2024
25f7d64
Recursive search for include files in non-header files
HerrCai0907 Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 32 additions & 15 deletions clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,28 @@ namespace {

AST_MATCHER(Decl, isFirstDecl) { return Node.isFirstDecl(); }

AST_MATCHER_P(Decl, isInMainFile, FileExtensionsSet, HeaderFileExtensions) {
static bool isInMainFile(SourceLocation L, SourceManager &SM,
const FileExtensionsSet &HeaderFileExtensions) {
for (;;) {
if (utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions))
return false;
if (SM.isInMainFile(L))
return true;
// not in header file but not in main file
L = SM.getIncludeLoc(SM.getFileID(L));
if (L.isValid())
continue;
// Conservative about the unknown
return false;
}
}

AST_MATCHER_P(Decl, isAllRedeclsInMainFile, 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);
return isInMainFile(D->getLocation(),
Finder->getASTContext().getSourceManager(),
HeaderFileExtensions);
Comment on lines +46 to +48
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

});
}

Expand All @@ -42,16 +58,17 @@ AST_POLYMORPHIC_MATCHER(isExternStorageClass,
} // namespace

void UseInternalLinkageCheck::registerMatchers(MatchFinder *Finder) {
auto Common = allOf(isFirstDecl(), isInMainFile(HeaderFileExtensions),
unless(anyOf(
// 1. internal linkage
isStaticStorageClass(), isInAnonymousNamespace(),
// 2. explicit external linkage
isExternStorageClass(), isExternC(),
// 3. template
isExplicitTemplateSpecialization(),
// 4. friend
hasAncestor(friendDecl()))));
auto Common =
allOf(isFirstDecl(), isAllRedeclsInMainFile(HeaderFileExtensions),
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"),
Expand Down
2 changes: 2 additions & 0 deletions clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/use-internal-linkage/func.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#pragma once

void func_header();

#include "func_h.inc"
1 change: 1 addition & 0 deletions clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/use-internal-linkage/func_cpp.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
void func_cpp_inc();
1 change: 1 addition & 0 deletions clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/use-internal-linkage/func_h.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
void func_h_inc();
7 changes: 7 additions & 0 deletions clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-func.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ template<class T>
void func_template() {}
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'func_template'

void func_cpp_inc();
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'func_cpp_inc'

#include "func_cpp.inc"

void func_h_inc();

struct S {
void method();
};
Expand Down
Loading