[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-07 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 closed 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-06 Thread Congcong Cai via cfe-commits 


@@ -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 ,
+ const FileExtensionsSet ) {
+  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  = 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);

HerrCai0907 wrote:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-06 Thread Congcong Cai via cfe-commits 


@@ -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 ,
+ const FileExtensionsSet ) {
+  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  = 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);

HerrCai0907 wrote:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-06 Thread Piotr Zegar via cfe-commits 


@@ -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 ,
+ const FileExtensionsSet ) {
+  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  = 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);

PiotrZSL wrote:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-06 Thread Piotr Zegar via cfe-commits 


@@ -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 ,
+ const FileExtensionsSet ) {
+  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  = 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);

PiotrZSL wrote:

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

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-06 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL deleted 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-06 Thread Piotr Zegar via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

PiotrZSL wrote:

No need to do recursive, using (current) first declaration should also work.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-06 Thread Congcong Cai via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

HerrCai0907 wrote:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-05 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL approved this pull request.

I accept this change as it's fine for 1.0 version
Any fixed can be always done in this or new change.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-05 Thread Piotr Zegar via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

PiotrZSL wrote:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-04 Thread Congcong Cai via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

HerrCai0907 wrote:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-04 Thread Piotr Zegar via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

PiotrZSL wrote:

simply .cpp (source files) files included from main file should be treated like 
main file.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-06-04 Thread Congcong Cai via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

HerrCai0907 wrote:

what is the expected behavior for unity files? use static or not? I am a little 
bit confused.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-28 Thread Piotr Zegar via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

PiotrZSL wrote:

this still won't work for "unity files" (includes .cpp)

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-28 Thread Piotr Zegar via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

PiotrZSL wrote:

I'm fine for lack of fixes in header files.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-26 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 updated 
https://github.com/llvm/llvm-project/pull/90830

>From 24cbbd0c87ab2a06381d210da1dff5f966b72773 Mon Sep 17 00:00:00 2001
From: Congcong Cai 
Date: Thu, 2 May 2024 15:44:45 +0800
Subject: [PATCH 1/7] reformat

---
 .../clang-tidy/readability/CMakeLists.txt |  1 +
 .../readability/ReadabilityTidyModule.cpp |  3 +
 .../UnnecessaryExternalLinkageCheck.cpp   | 82 +++
 .../UnnecessaryExternalLinkageCheck.h | 33 
 clang-tools-extra/docs/ReleaseNotes.rst   |  5 ++
 .../docs/clang-tidy/checks/list.rst   |  1 +
 .../unnecessary-external-linkage.rst  | 26 ++
 .../readability/Inputs/mark-static-var/func.h |  3 +
 .../readability/Inputs/mark-static-var/var.h  |  3 +
 .../unnecessary-external-linkage-func.cpp | 30 +++
 .../unnecessary-external-linkage-var.cpp  | 40 +
 11 files changed, 227 insertions(+)
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.h
 create mode 100644 
clang-tools-extra/docs/clang-tidy/checks/readability/unnecessary-external-linkage.rst
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-func.cpp
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-var.cpp

diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt 
b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 41065fc8e8785..8f58d9f24ba49 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
+  UnnecessaryExternalLinkageCheck.cpp
   RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp 
b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e..d389287e8f490 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -58,6 +58,7 @@
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryExternalLinkageCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
 #include "UseStdMinMaxCheck.h"
@@ -106,6 +107,8 @@ class ReadabilityModule : public ClangTidyModule {
 "readability-identifier-naming");
 CheckFactories.registerCheck(
 "readability-implicit-bool-conversion");
+CheckFactories.registerCheck(
+"readability-unnecessary-external-linkage");
 CheckFactories.registerCheck(
 "readability-math-missing-parentheses");
 CheckFactories.registerCheck(
diff --git 
a/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
new file mode 100644
index 0..4970d3339ef05
--- /dev/null
+++ 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
@@ -0,0 +1,82 @@
+//===--- UnnecessaryExternalLinkageCheck.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 "UnnecessaryExternalLinkageCheck.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) {
+  for (const Decl *D : Node.redecls())
+if (!Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation()))
+  return false;
+  return true;
+}
+
+AST_POLYMORPHIC_MATCHER(isExternStorageClass,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-23 Thread Congcong Cai via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

HerrCai0907 wrote:

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.
```c++
/// a.hpp
PREFIX int f() {
  static int a = 1;
  return a++;
}
void b();

/// a.cpp
#include "a.hpp"
#include 

int main() {
  std::cout << __func__ << " " << f() << "\n";
  b();
  std::cout << __func__ << " " << f() << "\n";
  b();
}

/// b.cpp
#include "a.hpp"
#include 

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 `, result is `1 2 3 4`.
if `PREFIX` is `template  static`, result is `1 2 1 2`.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-21 Thread Piotr Zegar via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

PiotrZSL wrote:

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

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Congcong Cai via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

HerrCai0907 wrote:

It will match more cases which I cannot understand the code style.
e.g.
```c++
/// a.cpp
void f(); // decl-1

/// b.cpp
#include 
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.


https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Congcong Cai via cfe-commits 


@@ -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  = 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),

HerrCai0907 wrote:

It still work fine. The reason I add `isFirstDecl` is that the check will check 
all redecl and add this limitation can avoid O(n^2) complexity.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 updated 
https://github.com/llvm/llvm-project/pull/90830

>From 24cbbd0c87ab2a06381d210da1dff5f966b72773 Mon Sep 17 00:00:00 2001
From: Congcong Cai 
Date: Thu, 2 May 2024 15:44:45 +0800
Subject: [PATCH 1/7] reformat

---
 .../clang-tidy/readability/CMakeLists.txt |  1 +
 .../readability/ReadabilityTidyModule.cpp |  3 +
 .../UnnecessaryExternalLinkageCheck.cpp   | 82 +++
 .../UnnecessaryExternalLinkageCheck.h | 33 
 clang-tools-extra/docs/ReleaseNotes.rst   |  5 ++
 .../docs/clang-tidy/checks/list.rst   |  1 +
 .../unnecessary-external-linkage.rst  | 26 ++
 .../readability/Inputs/mark-static-var/func.h |  3 +
 .../readability/Inputs/mark-static-var/var.h  |  3 +
 .../unnecessary-external-linkage-func.cpp | 30 +++
 .../unnecessary-external-linkage-var.cpp  | 40 +
 11 files changed, 227 insertions(+)
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.h
 create mode 100644 
clang-tools-extra/docs/clang-tidy/checks/readability/unnecessary-external-linkage.rst
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-func.cpp
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-var.cpp

diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt 
b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 41065fc8e8785..8f58d9f24ba49 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
+  UnnecessaryExternalLinkageCheck.cpp
   RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp 
b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e..d389287e8f490 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -58,6 +58,7 @@
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryExternalLinkageCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
 #include "UseStdMinMaxCheck.h"
@@ -106,6 +107,8 @@ class ReadabilityModule : public ClangTidyModule {
 "readability-identifier-naming");
 CheckFactories.registerCheck(
 "readability-implicit-bool-conversion");
+CheckFactories.registerCheck(
+"readability-unnecessary-external-linkage");
 CheckFactories.registerCheck(
 "readability-math-missing-parentheses");
 CheckFactories.registerCheck(
diff --git 
a/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
new file mode 100644
index 0..4970d3339ef05
--- /dev/null
+++ 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
@@ -0,0 +1,82 @@
+//===--- UnnecessaryExternalLinkageCheck.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 "UnnecessaryExternalLinkageCheck.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) {
+  for (const Decl *D : Node.redecls())
+if (!Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation()))
+  return false;
+  return true;
+}
+
+AST_POLYMORPHIC_MATCHER(isExternStorageClass,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Piotr Zegar via cfe-commits 


@@ -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  = Finder->getASTContext().getSourceManager();
+const SourceLocation L = D->getLocation();
+return SM.isInMainFile(L) &&
+   !utils::isSpellingLocInHeaderFile(L, SM, HeaderFileExtensions);
+  });

PiotrZSL wrote:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Piotr Zegar via cfe-commits 


@@ -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  = 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 wrote:

isFirstDecl wont work if someone do:

```
void foo()
{
}
#include "foo.h"
```

but thats, fine, just would be good if documentation mention that it checks 
first declaration

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL commented:

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.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL edited 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-13 Thread Piotr Zegar via cfe-commits 

PiotrZSL wrote:

> > * The auto-fix should be configurable to choose `static` or anonymous 
> > namespace.
> 
> 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. WDYT? 
> @carlosgalvezp @PiotrZSL @EugeneZelenko @SimplyDanny

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.
I had an quick fix with adding `static`, and that worked fine, in some places 
code didn't compile so had to fix those by my self, but that was fine.

Add an option for "static / anonymous namespace", and generate fixes/hints 
accordingly.
You can use optional values, or enums and by default suggest one or other, and 
in such state you may not need to provide fixes. In other config state just 
provide fixes, even if that would mean wrapping every function in separate 
anonymous namespace or adding static. There allways can be other check or some 
clang-format option to merge multiple namespaces.
Do not reorder functions, and one can use other. Also static is safer as it's 
does not change scope, with namespace user may run into issues, but still fixes 
are needed. You can always mention in documentation that fixes are not perfect 
and manual intervention may be required. Even if check will do 80% of job, 
thats already huge help.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-06 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 updated 
https://github.com/llvm/llvm-project/pull/90830

>From 24cbbd0c87ab2a06381d210da1dff5f966b72773 Mon Sep 17 00:00:00 2001
From: Congcong Cai 
Date: Thu, 2 May 2024 15:44:45 +0800
Subject: [PATCH 1/7] reformat

---
 .../clang-tidy/readability/CMakeLists.txt |  1 +
 .../readability/ReadabilityTidyModule.cpp |  3 +
 .../UnnecessaryExternalLinkageCheck.cpp   | 82 +++
 .../UnnecessaryExternalLinkageCheck.h | 33 
 clang-tools-extra/docs/ReleaseNotes.rst   |  5 ++
 .../docs/clang-tidy/checks/list.rst   |  1 +
 .../unnecessary-external-linkage.rst  | 26 ++
 .../readability/Inputs/mark-static-var/func.h |  3 +
 .../readability/Inputs/mark-static-var/var.h  |  3 +
 .../unnecessary-external-linkage-func.cpp | 30 +++
 .../unnecessary-external-linkage-var.cpp  | 40 +
 11 files changed, 227 insertions(+)
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.h
 create mode 100644 
clang-tools-extra/docs/clang-tidy/checks/readability/unnecessary-external-linkage.rst
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-func.cpp
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-var.cpp

diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt 
b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 41065fc8e87859..8f58d9f24ba491 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
+  UnnecessaryExternalLinkageCheck.cpp
   RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp 
b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e5..d389287e8f4909 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -58,6 +58,7 @@
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryExternalLinkageCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
 #include "UseStdMinMaxCheck.h"
@@ -106,6 +107,8 @@ class ReadabilityModule : public ClangTidyModule {
 "readability-identifier-naming");
 CheckFactories.registerCheck(
 "readability-implicit-bool-conversion");
+CheckFactories.registerCheck(
+"readability-unnecessary-external-linkage");
 CheckFactories.registerCheck(
 "readability-math-missing-parentheses");
 CheckFactories.registerCheck(
diff --git 
a/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
new file mode 100644
index 00..4970d3339ef05d
--- /dev/null
+++ 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
@@ -0,0 +1,82 @@
+//===--- UnnecessaryExternalLinkageCheck.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 "UnnecessaryExternalLinkageCheck.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) {
+  for (const Decl *D : Node.redecls())
+if (!Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation()))
+  return false;
+  return true;
+}
+
+AST_POLYMORPHIC_MATCHER(isExternStorageClass,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-06 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 updated 
https://github.com/llvm/llvm-project/pull/90830

>From 24cbbd0c87ab2a06381d210da1dff5f966b72773 Mon Sep 17 00:00:00 2001
From: Congcong Cai 
Date: Thu, 2 May 2024 15:44:45 +0800
Subject: [PATCH 1/6] reformat

---
 .../clang-tidy/readability/CMakeLists.txt |  1 +
 .../readability/ReadabilityTidyModule.cpp |  3 +
 .../UnnecessaryExternalLinkageCheck.cpp   | 82 +++
 .../UnnecessaryExternalLinkageCheck.h | 33 
 clang-tools-extra/docs/ReleaseNotes.rst   |  5 ++
 .../docs/clang-tidy/checks/list.rst   |  1 +
 .../unnecessary-external-linkage.rst  | 26 ++
 .../readability/Inputs/mark-static-var/func.h |  3 +
 .../readability/Inputs/mark-static-var/var.h  |  3 +
 .../unnecessary-external-linkage-func.cpp | 30 +++
 .../unnecessary-external-linkage-var.cpp  | 40 +
 11 files changed, 227 insertions(+)
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.h
 create mode 100644 
clang-tools-extra/docs/clang-tidy/checks/readability/unnecessary-external-linkage.rst
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-func.cpp
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-var.cpp

diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt 
b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 41065fc8e87859..8f58d9f24ba491 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
+  UnnecessaryExternalLinkageCheck.cpp
   RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp 
b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e5..d389287e8f4909 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -58,6 +58,7 @@
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryExternalLinkageCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
 #include "UseStdMinMaxCheck.h"
@@ -106,6 +107,8 @@ class ReadabilityModule : public ClangTidyModule {
 "readability-identifier-naming");
 CheckFactories.registerCheck(
 "readability-implicit-bool-conversion");
+CheckFactories.registerCheck(
+"readability-unnecessary-external-linkage");
 CheckFactories.registerCheck(
 "readability-math-missing-parentheses");
 CheckFactories.registerCheck(
diff --git 
a/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
new file mode 100644
index 00..4970d3339ef05d
--- /dev/null
+++ 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
@@ -0,0 +1,82 @@
+//===--- UnnecessaryExternalLinkageCheck.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 "UnnecessaryExternalLinkageCheck.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) {
+  for (const Decl *D : Node.redecls())
+if (!Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation()))
+  return false;
+  return true;
+}
+
+AST_POLYMORPHIC_MATCHER(isExternStorageClass,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-06 Thread Congcong Cai via cfe-commits 


@@ -0,0 +1,79 @@
+//===--- 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 "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isFirstDecl,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+VarDecl)) {
+  return Node.isFirstDecl();
+}
+
+AST_MATCHER(Decl, isInMainFile) {
+  return llvm::all_of(Node.redecls(), [&](const Decl *D) {
+return Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation());
+  });
+}
+
+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(),
+  unless(anyOf(
+  // 1. internal linkage
+  isStaticStorageClass(), isInAnonymousNamespace(),
+  // 2. explicit external linkage
+  isExternStorageClass(), isExternC(),
+  // 3. template
+  isExplicitTemplateSpecialization(),
+  clang::ast_matchers::isTemplateInstantiation(),
+  // 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 ) {
+  if (const auto *FD = Result.Nodes.getNodeAs("fn")) {
+diag(FD->getLocation(), Message) << "function" << FD;
+return;
+  }
+  if (const auto *VD = Result.Nodes.getNodeAs("var")) {
+diag(VD->getLocation(), Message) << "variable" << VD;
+return;
+  }

HerrCai0907 wrote:

It won't. It does not provide the source range, then it will only mark the 
begin location instead of the whole source range. The warning would be like 
this:
```
a.cpp:3:3: warning: function 'f2' can be made static or moved into an anonymous 
namespace to enforce internal linkage [misc-use-internal-linkage]
3 | S f2(const S ) { return val; }
  |   ^
```

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-05 Thread Julian Schmidt via cfe-commits 

https://github.com/5chmidti edited 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-05 Thread Julian Schmidt via cfe-commits 

https://github.com/5chmidti edited 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-05 Thread Julian Schmidt via cfe-commits 


@@ -0,0 +1,79 @@
+//===--- 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 "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isFirstDecl,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+VarDecl)) {
+  return Node.isFirstDecl();
+}
+
+AST_MATCHER(Decl, isInMainFile) {
+  return llvm::all_of(Node.redecls(), [&](const Decl *D) {
+return Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation());
+  });
+}
+
+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(),
+  unless(anyOf(
+  // 1. internal linkage
+  isStaticStorageClass(), isInAnonymousNamespace(),
+  // 2. explicit external linkage
+  isExternStorageClass(), isExternC(),
+  // 3. template
+  isExplicitTemplateSpecialization(),
+  clang::ast_matchers::isTemplateInstantiation(),

5chmidti wrote:

Template instantiations will already be ignored because of the traversal kind

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-05 Thread Julian Schmidt via cfe-commits 

https://github.com/5chmidti commented:

Regarding unit build:
`google-global-names-in-headers` is one of the checks that check file 
extensions: 
https://github.com/llvm/llvm-project/blob/d33937b6236767137a1ec3393d0933f10eed4ffe/clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp#L42-L44

Nice that there is a check for this now, it looks like there are a few 
instances of this issue in LLVM as well.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-05 Thread Julian Schmidt via cfe-commits 

https://github.com/5chmidti edited 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-05 Thread Julian Schmidt via cfe-commits 


@@ -0,0 +1,79 @@
+//===--- 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 "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isFirstDecl,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+VarDecl)) {
+  return Node.isFirstDecl();
+}

5chmidti wrote:

You don't need to make this a polymorphic matcher, `isFirstDecl` is part of 
`clang::Decl`.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-05 Thread Julian Schmidt via cfe-commits 


@@ -0,0 +1,79 @@
+//===--- 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 "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isFirstDecl,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+VarDecl)) {
+  return Node.isFirstDecl();
+}
+
+AST_MATCHER(Decl, isInMainFile) {
+  return llvm::all_of(Node.redecls(), [&](const Decl *D) {
+return Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation());
+  });
+}
+
+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(),
+  unless(anyOf(
+  // 1. internal linkage
+  isStaticStorageClass(), isInAnonymousNamespace(),
+  // 2. explicit external linkage
+  isExternStorageClass(), isExternC(),
+  // 3. template
+  isExplicitTemplateSpecialization(),
+  clang::ast_matchers::isTemplateInstantiation(),
+  // 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 ) {
+  if (const auto *FD = Result.Nodes.getNodeAs("fn")) {
+diag(FD->getLocation(), Message) << "function" << FD;
+return;
+  }
+  if (const auto *VD = Result.Nodes.getNodeAs("var")) {
+diag(VD->getLocation(), Message) << "variable" << VD;
+return;
+  }

5chmidti wrote:

Please stream `SourceRange`s into the diagnostics. For the `FunctionDecl` you 
could use `SourceRange(FD->getBeginLoc(), FD->getTypeSpecEndLoc())`, which 
would not highlight the body (that would be quite noisy).

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 updated 
https://github.com/llvm/llvm-project/pull/90830

>From 24cbbd0c87ab2a06381d210da1dff5f966b72773 Mon Sep 17 00:00:00 2001
From: Congcong Cai 
Date: Thu, 2 May 2024 15:44:45 +0800
Subject: [PATCH 1/5] reformat

---
 .../clang-tidy/readability/CMakeLists.txt |  1 +
 .../readability/ReadabilityTidyModule.cpp |  3 +
 .../UnnecessaryExternalLinkageCheck.cpp   | 82 +++
 .../UnnecessaryExternalLinkageCheck.h | 33 
 clang-tools-extra/docs/ReleaseNotes.rst   |  5 ++
 .../docs/clang-tidy/checks/list.rst   |  1 +
 .../unnecessary-external-linkage.rst  | 26 ++
 .../readability/Inputs/mark-static-var/func.h |  3 +
 .../readability/Inputs/mark-static-var/var.h  |  3 +
 .../unnecessary-external-linkage-func.cpp | 30 +++
 .../unnecessary-external-linkage-var.cpp  | 40 +
 11 files changed, 227 insertions(+)
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.h
 create mode 100644 
clang-tools-extra/docs/clang-tidy/checks/readability/unnecessary-external-linkage.rst
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-func.cpp
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-var.cpp

diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt 
b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 41065fc8e87859..8f58d9f24ba491 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
+  UnnecessaryExternalLinkageCheck.cpp
   RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp 
b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e5..d389287e8f4909 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -58,6 +58,7 @@
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryExternalLinkageCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
 #include "UseStdMinMaxCheck.h"
@@ -106,6 +107,8 @@ class ReadabilityModule : public ClangTidyModule {
 "readability-identifier-naming");
 CheckFactories.registerCheck(
 "readability-implicit-bool-conversion");
+CheckFactories.registerCheck(
+"readability-unnecessary-external-linkage");
 CheckFactories.registerCheck(
 "readability-math-missing-parentheses");
 CheckFactories.registerCheck(
diff --git 
a/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
new file mode 100644
index 00..4970d3339ef05d
--- /dev/null
+++ 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
@@ -0,0 +1,82 @@
+//===--- UnnecessaryExternalLinkageCheck.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 "UnnecessaryExternalLinkageCheck.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) {
+  for (const Decl *D : Node.redecls())
+if (!Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation()))
+  return false;
+  return true;
+}
+
+AST_POLYMORPHIC_MATCHER(isExternStorageClass,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 updated 
https://github.com/llvm/llvm-project/pull/90830

>From 24cbbd0c87ab2a06381d210da1dff5f966b72773 Mon Sep 17 00:00:00 2001
From: Congcong Cai 
Date: Thu, 2 May 2024 15:44:45 +0800
Subject: [PATCH 1/4] reformat

---
 .../clang-tidy/readability/CMakeLists.txt |  1 +
 .../readability/ReadabilityTidyModule.cpp |  3 +
 .../UnnecessaryExternalLinkageCheck.cpp   | 82 +++
 .../UnnecessaryExternalLinkageCheck.h | 33 
 clang-tools-extra/docs/ReleaseNotes.rst   |  5 ++
 .../docs/clang-tidy/checks/list.rst   |  1 +
 .../unnecessary-external-linkage.rst  | 26 ++
 .../readability/Inputs/mark-static-var/func.h |  3 +
 .../readability/Inputs/mark-static-var/var.h  |  3 +
 .../unnecessary-external-linkage-func.cpp | 30 +++
 .../unnecessary-external-linkage-var.cpp  | 40 +
 11 files changed, 227 insertions(+)
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
 create mode 100644 
clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.h
 create mode 100644 
clang-tools-extra/docs/clang-tidy/checks/readability/unnecessary-external-linkage.rst
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/func.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/mark-static-var/var.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-func.cpp
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/readability/unnecessary-external-linkage-var.cpp

diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt 
b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 41065fc8e87859..8f58d9f24ba491 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
+  UnnecessaryExternalLinkageCheck.cpp
   RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp 
b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e5..d389287e8f4909 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -58,6 +58,7 @@
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
+#include "UnnecessaryExternalLinkageCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
 #include "UseStdMinMaxCheck.h"
@@ -106,6 +107,8 @@ class ReadabilityModule : public ClangTidyModule {
 "readability-identifier-naming");
 CheckFactories.registerCheck(
 "readability-implicit-bool-conversion");
+CheckFactories.registerCheck(
+"readability-unnecessary-external-linkage");
 CheckFactories.registerCheck(
 "readability-math-missing-parentheses");
 CheckFactories.registerCheck(
diff --git 
a/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
new file mode 100644
index 00..4970d3339ef05d
--- /dev/null
+++ 
b/clang-tools-extra/clang-tidy/readability/UnnecessaryExternalLinkageCheck.cpp
@@ -0,0 +1,82 @@
+//===--- UnnecessaryExternalLinkageCheck.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 "UnnecessaryExternalLinkageCheck.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) {
+  for (const Decl *D : Node.redecls())
+if (!Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation()))
+  return false;
+  return true;
+}
+
+AST_POLYMORPHIC_MATCHER(isExternStorageClass,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread via cfe-commits 


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

EugeneZelenko wrote:

I'm not sure about automatic fixes for anonymous namespaces. For example, in my 
work project we keep single monolithic anonymous namespace, so function body 
move would be required.

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread Danny Mösch via cfe-commits 


@@ -0,0 +1,79 @@
+//===--- 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 "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isFirstDecl,
+AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+VarDecl)) {
+  return Node.isFirstDecl();
+}
+
+AST_MATCHER(Decl, isInMainFile) {
+  return llvm::all_of(Node.redecls(), [&](const Decl *D) {
+return Finder->getASTContext().getSourceManager().isInMainFile(
+D->getLocation());
+  });
+}
+
+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(),
+  unless(anyOf(
+  // 1. internal linkage
+  isStaticStorageClass(), isInAnonymousNamespace(),
+  // 2. explicit external linkage
+  isExternStorageClass(), isExternC(),
+  // 3. template
+  isExplicitTemplateSpecialization(),
+  clang::ast_matchers::isTemplateInstantiation(),
+  // 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 can be made static or moved into an anonymous namespace "
+"to enforce internal linkage.";

SimplyDanny wrote:

Typically, messages don't end with a period:
```suggestion
"%0 %1 can be made static or moved into an anonymous namespace "
"to enforce internal linkage";
```

https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread Danny Mösch via cfe-commits 

https://github.com/SimplyDanny approved this pull request.


https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread Danny Mösch via cfe-commits 

https://github.com/SimplyDanny edited 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 edited 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] new check misc-use-internal-linkage (PR #90830)

2024-05-04 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 edited 
https://github.com/llvm/llvm-project/pull/90830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits