[PATCH] D148793: [WIP][clang-tidy] Implement an include-cleaner check.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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