clang tidy: add header ignore option to include duplicate files #167046
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
Member
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Lakshdeep Singh (lakshsidhu04) ChangesAdded an option to enable users to include duplicate headers which match with certain regexes Full diff: https://github.com/llvm/llvm-project/pull/167046.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
index a0e7ac19ab2d5..bf0d9a7598679 100644
--- a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
@@ -107,7 +107,7 @@ class CyclicDependencyCallbacks : public PPCallbacks {
Check.diag(Loc, "direct self-inclusion of header file '%0'") << FileName;
return;
}
-
+
Check.diag(Loc, "circular header file dependency detected while including "
"'%0', please check the include path")
<< FileName;
diff --git a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
index 0237c057afed5..5ed783bcef824 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
@@ -11,7 +11,12 @@
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Regex.h"
+#include "llvm/Support/Path.h"
+#include "../utils/OptionsUtils.h"
+#include "llvm/ADT/StringExtras.h"
#include <memory>
+#include <vector>
namespace clang::tidy::readability {
@@ -33,12 +38,9 @@ using FileList = SmallVector<StringRef>;
class DuplicateIncludeCallbacks : public PPCallbacks {
public:
DuplicateIncludeCallbacks(DuplicateIncludeCheck &Check,
- const SourceManager &SM)
- : Check(Check), SM(SM) {
- // The main file doesn't participate in the FileChanged notification.
- Files.emplace_back();
- }
-
+ const SourceManager &SM,
+ const std::vector<std::string> &AllowedStrings);
+
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
FileID PrevFID) override;
@@ -62,14 +64,28 @@ class DuplicateIncludeCallbacks : public PPCallbacks {
SmallVector<FileList> Files;
DuplicateIncludeCheck &Check;
const SourceManager &SM;
+ std::vector<llvm::Regex> AllowedDuplicateRegex;
+
+ bool IsAllowedDuplicateInclude(StringRef TokenName, OptionalFileEntryRef File,
+ StringRef RelativePath);
};
} // namespace
-void DuplicateIncludeCallbacks::FileChanged(SourceLocation Loc,
- FileChangeReason Reason,
- SrcMgr::CharacteristicKind FileType,
- FileID PrevFID) {
+DuplicateIncludeCallbacks::DuplicateIncludeCallbacks(
+ DuplicateIncludeCheck &Check, const SourceManager &SM,
+ const std::vector<std::string> &AllowedStrings) : Check(Check), SM(SM) {
+ // The main file doesn't participate in the FileChanged notification.
+ Files.emplace_back();
+ AllowedDuplicateRegex.reserve(AllowedStrings.size());
+ for (const std::string &str : AllowedStrings) {
+ AllowedDuplicateRegex.emplace_back(str);
+ }
+}
+
+void DuplicateIncludeCallbacks::FileChanged(
+ SourceLocation Loc, FileChangeReason Reason,
+ SrcMgr::CharacteristicKind FileType, FileID PrevFID) {
if (Reason == EnterFile)
Files.emplace_back();
else if (Reason == ExitFile)
@@ -85,6 +101,14 @@ void DuplicateIncludeCallbacks::InclusionDirective(
if (FilenameRange.getBegin().isMacroID() ||
FilenameRange.getEnd().isMacroID())
return;
+
+ // if duplicate allowed, record and return
+ if(IsAllowedDuplicateInclude(FileName, File, RelativePath))
+ {
+ Files.back().push_back(FileName);
+ return;
+ }
+
if (llvm::is_contained(Files.back(), FileName)) {
// We want to delete the entire line, so make sure that [Start,End] covers
// everything.
@@ -109,9 +133,54 @@ void DuplicateIncludeCallbacks::MacroUndefined(const Token &MacroNameTok,
Files.back().clear();
}
+bool DuplicateIncludeCallbacks::IsAllowedDuplicateInclude(StringRef TokenName,
+ OptionalFileEntryRef File,
+ StringRef RelativePath) {
+ SmallVector<StringRef, 3> matchArguments;
+ matchArguments.push_back(TokenName);
+
+ if (!RelativePath.empty())
+ matchArguments.push_back(llvm::sys::path::filename(RelativePath));
+
+ if (File) {
+ StringRef RealPath = File->getFileEntry().tryGetRealPathName();
+ if (!RealPath.empty())
+ matchArguments.push_back(llvm::sys::path::filename(RealPath));
+ }
+
+ // try to match with each regex
+ for (const llvm::Regex ® : AllowedDuplicateRegex) {
+ for (StringRef arg : matchArguments) {
+ if (reg.match(arg))
+ return true;
+ }
+ }
+ return false;
+}
+
+DuplicateIncludeCheck::DuplicateIncludeCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {
+ std::string Raw = Options.get("AllowedDuplicateIncludes", "").str();
+ if (!Raw.empty()) {
+ SmallVector<StringRef, 4> StringParts;
+ StringRef(Raw).split(StringParts, ',', -1, false);
+
+ for (StringRef Part : StringParts) {
+ Part = Part.trim();
+ if (!Part.empty())
+ AllowedDuplicateIncludes.push_back(Part.str());
+ }
+ }
+}
+
void DuplicateIncludeCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
- PP->addPPCallbacks(std::make_unique<DuplicateIncludeCallbacks>(*this, SM));
+ PP->addPPCallbacks(std::make_unique<DuplicateIncludeCallbacks>(*this, SM, AllowedDuplicateIncludes));
}
+void DuplicateIncludeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "AllowedDuplicateIncludes",
+ llvm::join(AllowedDuplicateIncludes, ","));
+}
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
index 297999cf4f921..688c2ad4d30f1 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
@@ -10,7 +10,8 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATE_INCLUDE_CHECK_H
#include "../ClangTidyCheck.h"
-
+#include <vector>
+#include <string>
namespace clang::tidy::readability {
/// \brief Find and remove duplicate #include directives.
@@ -19,11 +20,14 @@ namespace clang::tidy::readability {
/// directives between them are analyzed.
class DuplicateIncludeCheck : public ClangTidyCheck {
public:
- DuplicateIncludeCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ DuplicateIncludeCheck(StringRef Name, ClangTidyContext *Context);
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+private:
+ std::vector<std::string> AllowedDuplicateIncludes;
};
} // namespace clang::tidy::readability
|
Author
|
Hi, i am relatively new to open source. Can you tell me if I need to make documentation changes if any? |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp -- clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/other.h clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/pack_begin.h clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/pack_end.h clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-allowed.cpp clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
index bf0d9a759..a0e7ac19a 100644
--- a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
@@ -107,7 +107,7 @@ public:
Check.diag(Loc, "direct self-inclusion of header file '%0'") << FileName;
return;
}
-
+
Check.diag(Loc, "circular header file dependency detected while including "
"'%0', please check the include path")
<< FileName;
|
Contributor
Hi, yes: you need to add 1)release notes, 2)tests and 3)documentation about new option. General advice: look at how other people do PRs that got merged - there you can find answers by yourself. Please try to make robust test cases and cover all edge-cases that you can think of! |
You can test this locally with the following command:git diff -U0 origin/main...HEAD -- clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h |
python3 clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \
-path build -p1 -quietView the output from clang-tidy here. |
vbvictor
reviewed
Nov 8, 2025
Comment on lines
37
to
53
| Option: ``AllowedDuplicateIncludes`` | ||
| ------------------------------------ | ||
|
|
||
| Headers listed in this option are exempt from warnings. For example: | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| -config='{CheckOptions: [{key: readability-duplicate-include.AllowedDuplicateIncludes, value: "pack_begin.h,pack_end.h"}]}' | ||
|
|
||
| This allows regex matches with ``pack_begin.h`` and ``pack_end.h`` to be included multiple times | ||
| without triggering diagnostics. | ||
|
|
||
| Notes | ||
| ----- | ||
|
|
||
| - Only direct includes in the current translation unit are checked. | ||
| - Useful for removing redundant includes and improving compile times in large codebases. |
Contributor
There was a problem hiding this comment.
Wrong option formatting, please see how other documents with options are formatted
| Option: ``AllowedDuplicateIncludes`` | ||
| ------------------------------------ | ||
|
|
||
| Headers listed in this option are exempt from warnings. For example: |
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/other.h
Outdated
Show resolved
Hide resolved
| #include "assertion.h" | ||
| // ...code with assertions disabled | ||
|
|
||
| Option: ``AllowedDuplicateIncludes`` |
Contributor
There was a problem hiding this comment.
Let's name it IgnoreHeaders, see https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html.
Just copy documentation from there
| std::string Raw = Options.get("AllowedDuplicateIncludes", "").str(); | ||
| if (!Raw.empty()) { | ||
| SmallVector<StringRef, 4> StringParts; | ||
| StringRef(Raw).split(StringParts, ',', -1, false); |
Contributor
There was a problem hiding this comment.
Should be semicolon-separated, see https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html#cmdoption-arg-IgnoreHeaders.
vbvictor
reviewed
Nov 8, 2025
Comment on lines
142
to
149
| if (!RelativePath.empty()) | ||
| matchArguments.push_back(llvm::sys::path::filename(RelativePath)); | ||
|
|
||
| if (File) { | ||
| StringRef RealPath = File->getFileEntry().tryGetRealPathName(); | ||
| if (!RealPath.empty()) | ||
| matchArguments.push_back(llvm::sys::path::filename(RealPath)); | ||
| } |
Contributor
There was a problem hiding this comment.
Why do we need 3 separate matchArguments to match?
To my understanding when we encounter:
#include "pack_begin.h"
#include "pack_begin.h" We only need to match only header name, what are other 2 matchArguments?
lakshsidhu04
force-pushed
the
duplicate-header-option
branch
from
0658b02 to
b611038
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added an option to enable users to include duplicate headers which match with certain regexes