[PATCH] D153340: [include-cleaner] Add an IgnoreHeaders flag to the command-line tool.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:138
+  /// Returns the path (without surrounding quotes/brackets) for the header.
+  /// For physical headers, this is the resolved path.
+  llvm::StringRef resolvedPath() const;
----------------
i think name and the comment are somewhat confusing, what about:
```
/// Absolute path for the header, when it's a physical file. Otherwise just the spelling without surrounding quotes/brackets.
```


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:82
+                 Ref.RT == RefType::Explicit &&
+                 (!HeaderFilter ||
+                  !HeaderFilter(Providers.front().resolvedPath())))
----------------
nit: rather than checking this at every use, might be easier to have something like:
```
if (!HeaderFilter)
  HeaderFilter = +[](llvm::StringRef) { return false; };
```


================
Comment at: clang-tools-extra/include-cleaner/test/tool.cpp:17
 
+//        RUN: clang-include-cleaner -print=changes %s --ignore-headers="foobar\.h,foo\.h" -- -I%S/Inputs/ | FileCheck --match-full-lines --allow-empty --check-prefix=IGNORE %s
+// IGNORE-NOT: - "foobar.h"
----------------
hokein wrote:
> kadircet wrote:
> > can you ignore one but keep other?
> > 
> > it'd be useful to also test the regex behaviour
> this tests aims to test filtering logic for both missing-includes and unused-includes cases.
> 
> added a new test.
we're still lacking a test for regex behaviour, maybe change next one to `foob.*\.h` ?


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:212
+private:
+  llvm::function_ref<bool(llvm::StringRef Path)> HeaderFilter;
+};
----------------
nit: Drop `Path`


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:231
+  }
+  return [FilterRegs](llvm::StringRef Path) {
+    llvm::errs() << "Path: " << Path << "\n";
----------------
`FilterRegs=std::move(FilterRegs)` and drop the `shared_ptr`?


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:232
+  return [FilterRegs](llvm::StringRef Path) {
+    llvm::errs() << "Path: " << Path << "\n";
+    for (const auto &F : *FilterRegs) {
----------------
looks like debugging artifact


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153340