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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:67
 /// Determine which headers should be inserted or removed from the main file.
 /// This exposes conclusions but not reasons: use lower-level walkUsed for that.
+AnalysisResults analyze(
----------------
can you add some comments about how header filter works? `A predicate that receives absolute path or spelling without quotes/brackets, when a phyiscal file doesn't exist. No analysis will be performed for headers that satisfy the predicate.`


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:72
+    const HeaderSearch &HS,
+    llvm::function_ref<bool(llvm::StringRef)> HeaderFilter =
+        [](llvm::StringRef HeaderPath) {
----------------
rather than a default here, can we just do no filtering when `HeaderFilter` is null ?


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:137
 
+  llvm::StringRef resolvedPath() const;
+
----------------
again some comments here could be useful


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:65
+        const HeaderSearch &HS,
+        llvm::function_ref<bool(llvm::StringRef Path)> HeaderFilter) {
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
----------------
can you drop `Path` or put it in comments


================
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"
----------------
can you ignore one but keep other?

it'd be useful to also test the regex behaviour


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:106
+public:
+  Action(llvm::function_ref<bool(llvm::StringRef Path)> HeaderFilter)
+      : HeaderFilter(HeaderFilter){};
----------------
same as above, can you either drop or comment out `Path`


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:113
   PragmaIncludes PI;
+  llvm::function_ref<bool(llvm::StringRef Path)> HeaderFilter;
 
----------------
same here for `Path`


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:157
       case PrintStyle::Changes:
-        for (const Include *I : Results.Unused)
+        for (const Include *I : Results.Unused) {
           llvm::outs() << "- " << I->quote() << " @Line:" << I->Line << "\n";
----------------
nit: braces


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:225
   }
+  std::vector<llvm::Regex> FilterRegs;
+  llvm::SmallVector<llvm::StringRef> Headers;
----------------
nit: might be better to extract this into a function like: `std::function<bool(StringRef)> headerFilter() { /* parses flags, returns a null function on failure */ }`


================
Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:227
+  llvm::SmallVector<llvm::StringRef> Headers;
+  llvm::StringRef(IgnoreHeaders).split(Headers, ',', -1, /*KeepEmpty*/ false);
+  for (auto HeaderPattern : Headers) {
----------------
s/KeepEmpty/KeepEmpty=/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153340