[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:61
+void IncludeCleanerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Suppress", "");
+}
----------------
after storing them in the class state on construction, we should provide them with current values here:
```
  /// Should store all options supported by this check with their
  /// current values or default values for options that haven't been overridden.
```


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:66
+  llvm::SmallVector<llvm::StringRef> SuppressedHeaders;
+  auto SuppressOpt = Options.getLocalOrGlobal("Suppress");
+  if (SuppressOpt)
----------------
as mentioned above, rather than doing this on demand, we should run the logic on construction and store in the class state


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:66
+  llvm::SmallVector<llvm::StringRef> SuppressedHeaders;
+  auto SuppressOpt = Options.getLocalOrGlobal("Suppress");
+  if (SuppressOpt)
----------------
kadircet wrote:
> as mentioned above, rather than doing this on demand, we should run the logic on construction and store in the class state
s/Suppress/IgnoreHeader

to be consistent with option in clangd


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:81
+       Result.Nodes.getNodeAs<TranslationUnitDecl>("top")->decls()) {
+    if (!SM->isInMainFile(D->getLocation()))
+      continue;
----------------
we should actually use FileLoc of the decl location here (i.e. map it back to spelling location) as the decl might be introduced by a macro expansion, but if the spelling of "primary" location belongs to the main file we should still analyze it (e.g. decls introduced via `TEST` macros)

also `SourceManager::isInMainFile` will follow `#line` directives, which can create confusion (a declaration from some other file will create a diagnostic in the main file), so let's use `isWrittenInMainFile` instead?


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:101
+        }
+        if (!Satisfied && !Providers.empty() &&
+            Ref.RT == include_cleaner::RefType::Explicit)
----------------
we should also respect `IgnoreHeaders` here


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:126
+
+    if (std::find(IgnoreHeaders.begin(), IgnoreHeaders.end(), I.Spelled) ==
+        IgnoreHeaders.end())
----------------
sorry i guess i wasn't explicit about this one, but it should actually be a suffix match based regex (e.g. `foo/.*` disables analysis for all sources under a directory called `foo`) on the resolved path of the include (similar to what we do in clangd).

as headers can be spelled differently based on the translation unit and this option is mentioned for the whole codebase.


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:131
+
+  llvm::StringRef FileData = SM->getBufferData(SM->getMainFileID());
+  auto FileStyle = format::getStyle(
----------------
`FileData` makes it sound like this is some other data about the file rather than its contents. maybe just `Code`?


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:144
+        HeaderIncludes.remove(Inc->Spelled, Inc->Angled);
+    tooling::Replacement R = *Removal.begin();
+    auto StartLoc = SM->getComposedLoc(SM->getMainFileID(), R.getOffset());
----------------
sorry I know I suggested using HeaderIncludes for removals too, but this seems to cause more trouble actually;

we should actually go over rest of the replacements too, HeaderIncludes will generate removals for all the includes that match this (spelling, quoting). hence we can't just apply the first removal.
but that'll imply we'll remove them multiple times (as analysis above will treat each of them as a separate unused include). hence we'll need some deduplicaiton.

it might be easier to just use line number we have in `Inc` and use `SourceManager:: translateFileLineCol` to drop the whole line (up until start of the next line).


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:43
+  HeaderSearch *HS;
+  llvm::SmallVector<llvm::StringRef> getSuppressedHeaders();
+};
----------------
the convention is to rather store options in check's state on construction. see documentation on ClangTidyCheck::ClangTidyCheck:

```
  /// Initializes the check with \p CheckName and \p Context.
  ///
  /// Derived classes must implement the constructor with this signature or
  /// delegate it. If a check needs to read options, it can do this in the
  /// constructor using the Options.get() methods below.
```


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:8
+the main file of a translation unit.
+Findings correspond to https://clangd.llvm.org/design/include-cleaner.
----------------
can you also mention the check options for ignoring analysis?


================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:18
+  const char *PreCode = R"(
+#include "bar.h"
+#include <vector>
----------------
can you have the includes multiple times and make sure all of them are being dropped?


================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:37
+  ClangTidyOptions Opts;
+  Opts.CheckOptions["Suppress"] = "bar.h,vector";
+  EXPECT_EQ(PreCode, runCheckOnCode<IncludeCleanerCheck>(
----------------
can you also test for insertion suppressions ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793