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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked 8 inline comments as done.
VitaNuo added a comment.

Thanks for the comments! I'll re-assign the review to Haojian for now, so that we can hopefully make more progress this week.



================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:48
+void IncludeCleanerCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
----------------
kadircet wrote:
> 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++.
Ah thanks for the tip with `isLanguageVersionSupported`, doing that now.


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:67
+  for (const auto &Header : IgnoreHeaders)
+    IgnoreHeadersRegex.push_back(llvm::Regex{Header});
+  return IgnoreHeadersRegex;
----------------
kadircet wrote:
> 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.
Thanks, good ideas!


================
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
----------------
kadircet wrote:
> 
You probably wanted to say `insertion/removal` instead of `inclusion/insertion`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148793/new/

https://reviews.llvm.org/D148793