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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
