-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy] Add IgnoredFilesList option to readability-duplicate-include
#168196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-clang-tidy Author: mitchell (zeyi2) ChangesCloses #166938 Full diff: https://github.com/llvm/llvm-project/pull/168196.diff 7 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
index cc9ae471a926d..6a7324c446cfe 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
@@ -7,10 +7,12 @@
//===----------------------------------------------------------------------===//
#include "DuplicateIncludeCheck.h"
+#include "../utils/OptionsUtils.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Regex.h"
#include <memory>
namespace clang::tidy::readability {
@@ -33,10 +35,17 @@ using FileList = SmallVector<StringRef>;
class DuplicateIncludeCallbacks : public PPCallbacks {
public:
DuplicateIncludeCallbacks(DuplicateIncludeCheck &Check,
- const SourceManager &SM)
+ const SourceManager &SM,
+ const std::vector<StringRef> &IgnoredList)
: Check(Check), SM(SM) {
// The main file doesn't participate in the FileChanged notification.
Files.emplace_back();
+
+ AllowedRegexes.reserve(IgnoredList.size());
+ for (const StringRef &It : IgnoredList) {
+ if (!It.empty())
+ AllowedRegexes.emplace_back(It);
+ }
}
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
@@ -62,10 +71,32 @@ class DuplicateIncludeCallbacks : public PPCallbacks {
SmallVector<FileList> Files;
DuplicateIncludeCheck &Check;
const SourceManager &SM;
+ std::vector<llvm::Regex> AllowedRegexes;
+
+ bool isAllowedDuplicate(StringRef FileName, OptionalFileEntryRef File) const {
+ if (llvm::any_of(AllowedRegexes,
+ [&](const llvm::Regex &R) { return R.match(FileName); }))
+ return true;
+
+ if (File) {
+ const StringRef Resolved = File->getName();
+ if (llvm::any_of(AllowedRegexes,
+ [&](const llvm::Regex &R) { return R.match(Resolved); }))
+ return true;
+ }
+
+ return false;
+ }
};
} // namespace
+DuplicateIncludeCheck::DuplicateIncludeCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IgnoredFilesList(utils::options::parseStringList(
+ Options.get("IgnoredFilesList", ""))) {}
+
void DuplicateIncludeCallbacks::FileChanged(SourceLocation Loc,
FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -86,6 +117,9 @@ void DuplicateIncludeCallbacks::InclusionDirective(
FilenameRange.getEnd().isMacroID())
return;
if (llvm::is_contained(Files.back(), FileName)) {
+ if (isAllowedDuplicate(FileName, File)) {
+ return;
+ }
// We want to delete the entire line, so make sure that [Start,End] covers
// everything.
const SourceLocation Start =
@@ -111,7 +145,13 @@ void DuplicateIncludeCallbacks::MacroUndefined(const Token &MacroNameTok,
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, IgnoredFilesList));
+}
+
+void DuplicateIncludeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IgnoredFilesList",
+ utils::options::serializeStringList(IgnoredFilesList));
}
} // 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 ca3679108e60b..816c280b1fbea 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATEINCLUDECHECK_H
#include "../ClangTidyCheck.h"
+#include <vector>
namespace clang::tidy::readability {
@@ -19,11 +20,16 @@ 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:
+ // Semicolon-separated list of regexes or file names to ignore from duplicate
+ // warnings.
+ const std::vector<StringRef> IgnoredFilesList;
};
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4096624923858..52ffc0f82537b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -316,7 +316,7 @@ Changes in existing checks
exceptions from captures are now diagnosed, exceptions in the bodies of
lambdas that aren't actually invoked are not. Additionally, fixed an issue
where the check wouldn't diagnose throws in arguments to functions or
- constructors. Added fine-grained configuration via options
+ constructors. Added fine-grained configuration via options
`CheckDestructors`, `CheckMoveMemberFunctions`, `CheckMain`,
`CheckedSwapFunctions`, and `CheckNothrowFunctions`.
@@ -505,6 +505,11 @@ Changes in existing checks
ignoring default constructors with user provided arguments and adding
detection in container's method except ``empty``.
+- Improved :doc:`readability-duplicate-include
+ <clang-tidy/checks/readability/duplicate-include>` check by adding
+ the ``IgnoredFilesList`` option (semicolon-separated list of regexes or
+ filenames) to allow intentional duplicates.
+
- Improved :doc:`readability-identifier-naming
<clang-tidy/checks/readability/identifier-naming>` check by ignoring
declarations and macros in system headers. The documentation is also improved
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
index 45df7e1b84f3f..b2b03a51b25e8 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
@@ -7,6 +7,17 @@ Looks for duplicate includes and removes them. The check maintains a list of
included files and looks for duplicates. If a macro is defined or undefined
then the list of included files is cleared.
+Options
+-------
+
+.. option:: IgnoredFilesList
+
+ A semicolon-separated list of regular expressions or filenames that are
+ allowed to be included multiple times without diagnostics. Matching is
+ performed against the textual include name. If the header can be
+ resolved, its full path is also matched against the patterns.
+ Default is empty.
+
Examples:
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_begin.h b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_begin.h
new file mode 100644
index 0000000000000..fc9369c0cfb8f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_begin.h
@@ -0,0 +1 @@
+// Intentionally unguarded begin-pack header used in tests
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_end.h b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_end.h
new file mode 100644
index 0000000000000..78fd0a9adf475
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_end.h
@@ -0,0 +1 @@
+// Intentionally unguarded end-pack header used in tests
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp
new file mode 100644
index 0000000000000..91fe1bbb70234
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy %s readability-duplicate-include %t -- \
+// RUN: -config="{CheckOptions: {readability-duplicate-include.IgnoredFilesList: 'pack_.*\\.h'}}" \
+// RUN: -- -I %S/Inputs/duplicate-include
+
+#include "pack_begin.h"
+struct A { int x; };
+#include "pack_end.h"
+
+// no warning
|
|
@llvm/pr-subscribers-clang-tools-extra Author: mitchell (zeyi2) ChangesCloses #166938 Full diff: https://github.com/llvm/llvm-project/pull/168196.diff 7 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
index cc9ae471a926d..6a7324c446cfe 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
@@ -7,10 +7,12 @@
//===----------------------------------------------------------------------===//
#include "DuplicateIncludeCheck.h"
+#include "../utils/OptionsUtils.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Regex.h"
#include <memory>
namespace clang::tidy::readability {
@@ -33,10 +35,17 @@ using FileList = SmallVector<StringRef>;
class DuplicateIncludeCallbacks : public PPCallbacks {
public:
DuplicateIncludeCallbacks(DuplicateIncludeCheck &Check,
- const SourceManager &SM)
+ const SourceManager &SM,
+ const std::vector<StringRef> &IgnoredList)
: Check(Check), SM(SM) {
// The main file doesn't participate in the FileChanged notification.
Files.emplace_back();
+
+ AllowedRegexes.reserve(IgnoredList.size());
+ for (const StringRef &It : IgnoredList) {
+ if (!It.empty())
+ AllowedRegexes.emplace_back(It);
+ }
}
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
@@ -62,10 +71,32 @@ class DuplicateIncludeCallbacks : public PPCallbacks {
SmallVector<FileList> Files;
DuplicateIncludeCheck &Check;
const SourceManager &SM;
+ std::vector<llvm::Regex> AllowedRegexes;
+
+ bool isAllowedDuplicate(StringRef FileName, OptionalFileEntryRef File) const {
+ if (llvm::any_of(AllowedRegexes,
+ [&](const llvm::Regex &R) { return R.match(FileName); }))
+ return true;
+
+ if (File) {
+ const StringRef Resolved = File->getName();
+ if (llvm::any_of(AllowedRegexes,
+ [&](const llvm::Regex &R) { return R.match(Resolved); }))
+ return true;
+ }
+
+ return false;
+ }
};
} // namespace
+DuplicateIncludeCheck::DuplicateIncludeCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IgnoredFilesList(utils::options::parseStringList(
+ Options.get("IgnoredFilesList", ""))) {}
+
void DuplicateIncludeCallbacks::FileChanged(SourceLocation Loc,
FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -86,6 +117,9 @@ void DuplicateIncludeCallbacks::InclusionDirective(
FilenameRange.getEnd().isMacroID())
return;
if (llvm::is_contained(Files.back(), FileName)) {
+ if (isAllowedDuplicate(FileName, File)) {
+ return;
+ }
// We want to delete the entire line, so make sure that [Start,End] covers
// everything.
const SourceLocation Start =
@@ -111,7 +145,13 @@ void DuplicateIncludeCallbacks::MacroUndefined(const Token &MacroNameTok,
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, IgnoredFilesList));
+}
+
+void DuplicateIncludeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IgnoredFilesList",
+ utils::options::serializeStringList(IgnoredFilesList));
}
} // 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 ca3679108e60b..816c280b1fbea 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATEINCLUDECHECK_H
#include "../ClangTidyCheck.h"
+#include <vector>
namespace clang::tidy::readability {
@@ -19,11 +20,16 @@ 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:
+ // Semicolon-separated list of regexes or file names to ignore from duplicate
+ // warnings.
+ const std::vector<StringRef> IgnoredFilesList;
};
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4096624923858..52ffc0f82537b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -316,7 +316,7 @@ Changes in existing checks
exceptions from captures are now diagnosed, exceptions in the bodies of
lambdas that aren't actually invoked are not. Additionally, fixed an issue
where the check wouldn't diagnose throws in arguments to functions or
- constructors. Added fine-grained configuration via options
+ constructors. Added fine-grained configuration via options
`CheckDestructors`, `CheckMoveMemberFunctions`, `CheckMain`,
`CheckedSwapFunctions`, and `CheckNothrowFunctions`.
@@ -505,6 +505,11 @@ Changes in existing checks
ignoring default constructors with user provided arguments and adding
detection in container's method except ``empty``.
+- Improved :doc:`readability-duplicate-include
+ <clang-tidy/checks/readability/duplicate-include>` check by adding
+ the ``IgnoredFilesList`` option (semicolon-separated list of regexes or
+ filenames) to allow intentional duplicates.
+
- Improved :doc:`readability-identifier-naming
<clang-tidy/checks/readability/identifier-naming>` check by ignoring
declarations and macros in system headers. The documentation is also improved
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
index 45df7e1b84f3f..b2b03a51b25e8 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
@@ -7,6 +7,17 @@ Looks for duplicate includes and removes them. The check maintains a list of
included files and looks for duplicates. If a macro is defined or undefined
then the list of included files is cleared.
+Options
+-------
+
+.. option:: IgnoredFilesList
+
+ A semicolon-separated list of regular expressions or filenames that are
+ allowed to be included multiple times without diagnostics. Matching is
+ performed against the textual include name. If the header can be
+ resolved, its full path is also matched against the patterns.
+ Default is empty.
+
Examples:
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_begin.h b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_begin.h
new file mode 100644
index 0000000000000..fc9369c0cfb8f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_begin.h
@@ -0,0 +1 @@
+// Intentionally unguarded begin-pack header used in tests
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_end.h b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_end.h
new file mode 100644
index 0000000000000..78fd0a9adf475
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/pack_end.h
@@ -0,0 +1 @@
+// Intentionally unguarded end-pack header used in tests
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp
new file mode 100644
index 0000000000000..91fe1bbb70234
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy %s readability-duplicate-include %t -- \
+// RUN: -config="{CheckOptions: {readability-duplicate-include.IgnoredFilesList: 'pack_.*\\.h'}}" \
+// RUN: -- -I %S/Inputs/duplicate-include
+
+#include "pack_begin.h"
+struct A { int x; };
+#include "pack_end.h"
+
+// no warning
|
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-ignored-files.cpp
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
Outdated
Show resolved
Hide resolved
|
There is a PR doing same things: #167046. |
I think this PR is not actively being worked on, the issue got reassigned twice, please see #166938 (comment) |
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
🐧 Linux x64 Test Results
|
clang-tools-extra/docs/clang-tidy/checks/readability/duplicate-include.rst
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
Outdated
Show resolved
Hide resolved
vbvictor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #166938