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