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.

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
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
1 change: 1 addition & 0 deletions clang-tools-extra/clang-tidy/misc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ add_clang_library(clangTidyMiscModule
UnusedParametersCheck.cpp
UnusedUsingDeclsCheck.cpp
UseAnonymousNamespaceCheck.cpp
UseInternalLinkageCheck.cpp

LINK_LIBS
clangAnalysis
Expand Down
3 changes: 3 additions & 0 deletions clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "UnusedParametersCheck.h"
#include "UnusedUsingDeclsCheck.h"
#include "UseAnonymousNamespaceCheck.h"
#include "UseInternalLinkageCheck.h"

namespace clang::tidy {
namespace misc {
Expand Down Expand Up @@ -78,6 +79,8 @@ class MiscModule : public ClangTidyModule {
"misc-unused-using-decls");
CheckFactories.registerCheck<UseAnonymousNamespaceCheck>(
"misc-use-anonymous-namespace");
CheckFactories.registerCheck<UseInternalLinkageCheck>(
"misc-use-internal-linkage");
}
};

Expand Down
78 changes: 78 additions & 0 deletions clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
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);
});
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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:

  1. user want to run check on a header files, in such case header file will be a main file, and check wont work.
  2. 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).

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

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

}

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),
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;
}
llvm_unreachable("");
}

} // namespace clang::tidy::misc
38 changes: 38 additions & 0 deletions clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.h
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
6 changes: 6 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ New checks
Finds initializer lists for aggregate types that could be
written as designated initializers instead.

- New :doc:`misc-use-internal-linkage
<clang-tidy/checks/misc/use-internal-linkage>` check.

Detects variables and functions that can be marked as static or moved into
an anonymous namespace to enforce internal linkage.

- New :doc:`readability-enum-initial-value
<clang-tidy/checks/readability/enum-initial-value>` check.

Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/docs/clang-tidy/checks/list.rst
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ Clang-Tidy Checks
:doc:`misc-unused-parameters <misc/unused-parameters>`, "Yes"
:doc:`misc-unused-using-decls <misc/unused-using-decls>`, "Yes"
:doc:`misc-use-anonymous-namespace <misc/use-anonymous-namespace>`,
:doc:`misc-use-internal-linkage <misc/use-internal-linkage>`,
:doc:`modernize-avoid-bind <modernize/avoid-bind>`, "Yes"
:doc:`modernize-avoid-c-arrays <modernize/avoid-c-arrays>`,
:doc:`modernize-concat-nested-namespaces <modernize/concat-nested-namespaces>`, "Yes"
Expand Down
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;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#pragma once

void func_header();
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#pragma once

extern int gloabl_header;
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() {}
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;
Loading