kadircet added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:42
+
+struct MissingIncludeInfo {
+ SourceLocation SymRefLocation;
----------------
let's put this struct into anon namespace
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:48
+void IncludeCleanerCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+ return;
----------------
in theory this is not enough to disable the check for objc++, it'll have both
objc and cplusplus set. so we should check `ObjC` is false (there's no need to
disable it for `c`, right?)
nit: we can also override ` isLanguageVersionSupported` to disable check for
non-c++.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:67
+ for (const auto &Header : IgnoreHeaders)
+ IgnoreHeadersRegex.push_back(llvm::Regex{Header});
+ return IgnoreHeadersRegex;
----------------
let's make sure we're only going to match suffixes by appending `$` to the
`Header` here.
also before pushing into `IgnoreHeadersRegex` can you verify the regex
`isValid` and report a diagnostic via `configurationDiag` if regex is invalid.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:72
+void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto IgnoreHeadersRegex = parseIgnoreHeaderOptions(IgnoreHeadersOpt);
+ const SourceManager *SM = Result.SourceManager;
----------------
`check` is performed only once for this check so it doesn't matter, but it
might be better to perform this in constructor and store as class state to make
sure we're only parsing options and creating the regexes once (also possibly
reporting diagnostics once). as regex creation is actually expensive
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:83
+ continue;
+ MainFileDecls.push_back(const_cast<Decl *>(D));
+ }
----------------
instead of `const_cast` here, you can just drop the `const` in `const auto *D`
in `for` loop variable
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:107
+ IgnoreHeadersRegex, [&Spelled](const llvm::Regex &R) {
+ return R.match(llvm::StringRef(Spelled).trim("\"<>"));
+ }))
----------------
we're running this against spelled include of the file, not resolved path.
we should instead use the spelling + trimmed version for verbatim/standard
headers and use `FileEntry::tryGetRealPathName` for phyiscal headers. we can
even do the filtering before spelling the header to not spell redundantly.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:48
+ HeaderSearch *HS;
+ llvm::StringRef IgnoreHeadersOpt;
+};
----------------
let's make a full copy here and store a `std::string` instead, as reference
from options might become dangling. also the copy is cheap, we do that once per
check instantiation.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:15
+
+ A comma-separated list of header files that should be ignored during the
+ analysis. The option allows for regular expressions. E.g., `foo/.*` disables
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148793/new/
https://reviews.llvm.org/D148793
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
