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

[kadircet]

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

54

A comma-separated list of regexes to match against suffix of a header, and disable analysis if matched.

139

erroring out only after we performed all of our analysis and build a TU feels unfortunate. can we build these regexes and verify them at main and pass onto action instead? (in case the action is run on multiple files at once, we shouldn't compile/verify regexes over and over again)

160

we actually want to perform filtering inside analyze. it's not enough to filter only when printing, we should also filter before applying fixes :D. but we should also be applying filtering to absolute paths in all cases, for missing includes this is being applied to spelling instead.

[hokein]

address comments:

• move the filtering in analysis API
• add an actual -edit lit test

[hokein]

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

160

oops, good catch. Added an actual --edit test.

[kadircet]

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h

67 ↗

(On Diff #533896)

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.

72 ↗

(On Diff #533896)

rather than a default here, can we just do no filtering when HeaderFilter is null ?

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h

137 ↗

(On Diff #533896)

again some comments here could be useful

clang-tools-extra/include-cleaner/lib/Analysis.cpp

65 ↗

(On Diff #533896)

can you drop Path or put it in comments

clang-tools-extra/include-cleaner/test/tool.cpp

17

can you ignore one but keep other?

it'd be useful to also test the regex behaviour

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

104

same as above, can you either drop or comment out Path

106

same here for Path

170–172

nit: braces

230

nit: might be better to extract this into a function like: std::function<bool(StringRef)> headerFilter() { /* parses flags, returns a null function on failure */ }

232

s/KeepEmpty/KeepEmpty=/

[hokein]

address review comments.

[hokein]

clang-tools-extra/include-cleaner/test/tool.cpp

17

this tests aims to test filtering logic for both missing-includes and unused-includes cases.

added a new test.

[kadircet]

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h

138 ↗

(On Diff #536998)

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.

clang-tools-extra/include-cleaner/lib/Analysis.cpp

82 ↗

(On Diff #536998)

nit: rather than checking this at every use, might be easier to have something like:

if (!HeaderFilter)
  HeaderFilter = +[](llvm::StringRef) { return false; };

clang-tools-extra/include-cleaner/test/tool.cpp

17

we're still lacking a test for regex behaviour, maybe change next one to foob.*\.h ?

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

225

nit: Drop Path

244

FilterRegs=std::move(FilterRegs) and drop the shared_ptr?

245

looks like debugging artifact

[hokein]

address comments

[hokein]

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

244

This was my first attempt, it didn't work, we're returning a lamdba function, which invokes a delete constructor of llvm::Regex.

clang-tools-extra/include-cleaner/test/tool.cpp

 #include "foobar.h"
 
 int x = foo();
 
 //         RUN: clang-include-cleaner -print=changes %s -- -I%S/Inputs/ | FileCheck --check-prefix=CHANGE %s
 //      CHANGE: - "foobar.h"
 // CHANGE-NEXT: + "foo.h"
 
 //         RUN: clang-include-cleaner -remove=0 -print=changes %s -- -I%S/Inputs/ | FileCheck --check-prefix=INSERT %s
 //  INSERT-NOT: - "foobar.h"
 //      INSERT: + "foo.h"
 
 //         RUN: clang-include-cleaner -insert=0 -print=changes %s -- -I%S/Inputs/ | FileCheck --check-prefix=REMOVE %s
 //      REMOVE: - "foobar.h"
 //  REMOVE-NOT: + "foo.h"
 
+//        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"
+// IGNORE-NOT: + "foo.h"
+
 //        RUN: clang-include-cleaner -print %s -- -I%S/Inputs/ | FileCheck --match-full-lines --check-prefix=PRINT %s
 //      PRINT: #include "foo.h"
 //  PRINT-NOT: {{^}}#include "foobar.h"{{$}}
 
 //        RUN: cp %s %t.cpp
 //        RUN: clang-include-cleaner -edit %t.cpp -- -I%S/Inputs/
 //        RUN: FileCheck --match-full-lines --check-prefix=EDIT %s < %t.cpp
 //       EDIT: #include "foo.h"
 //   EDIT-NOT: {{^}}#include "foobar.h"{{$}}

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

 //===--- IncludeCleaner.cpp - standalone tool for include analysis --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AnalysisInternal.h"
 #include "clang-include-cleaner/Analysis.h"
 #include "clang-include-cleaner/Record.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace include_cleaner {
 namespace {
 namespace cl = llvm::cl;
 

 
 cl::opt<std::string> HTMLReportPath{
     "html",
     cl::desc("Specify an output filename for an HTML report. "
              "This describes both recommendations and reasons for changes."),
     cl::cat(IncludeCleaner),
 };
 
+cl::opt<std::string> IgnoreHeaders{
+    "ignore-headers",
+    cl::desc("A comma-separated list of headers to ignore."),
+    cl::init(""),
+    cl::cat(IncludeCleaner),
+};
+
 enum class PrintStyle { Changes, Final };
 cl::opt<PrintStyle> Print{
     "print",
     cl::values(
         clEnumValN(PrintStyle::Changes, "changes", "Print symbolic changes"),
         clEnumValN(PrintStyle::Final, "", "Print final code")),
     cl::ValueOptional,
     cl::init(PrintStyle::Final),
if (!S || !S->isCpp()) {
     consumeError(S.takeError());
     return format::getLLVMStyle();
   }
   return std::move(*S);
 }
 
 class Action : public clang::ASTFrontendAction {
   RecordedAST AST;
   RecordedPP PP;
   PragmaIncludes PI;
 
   void ExecuteAction() override {
     auto &P = getCompilerInstance().getPreprocessor();
     P.addPPCallbacks(PP.record(P));
     PI.record(getCompilerInstance());
     ASTFrontendAction::ExecuteAction();
   }
 
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef File) override {
     return AST.record();
   }
 
   void EndSourceFile() override {
     const auto &SM = getCompilerInstance().getSourceManager();
     if (SM.getDiagnostics().hasUncompilableErrorOccurred()) {
       llvm::errs()
           << "Skipping file " << getCurrentFile()
           << " due to compiler errors. clang-include-cleaner expects to "
              "work on compilable source code.\n";
       return;
     }
 
+    std::vector<llvm::Regex> HeaderFilters;
+    llvm::SmallVector<StringRef> Headers;
+    llvm::StringRef(IgnoreHeaders).split(Headers, ',', -1, /*KeepEmpty*/ false);
+    for (auto HeaderPattern : Headers) {
+      std::string AnchoredPattern = "(" + HeaderPattern.str() + ")$";
+      llvm::Regex CompiledRegex(AnchoredPattern);
+      std::string RegexError;
+      if (!CompiledRegex.isValid(RegexError)) {
+        llvm::errs() << llvm::formatv("Invalid regular expression '{0}': {1}\n",
+                                      HeaderPattern, RegexError);
+        return;
+      }
+      HeaderFilters.push_back(std::move(CompiledRegex));
+    }
+    auto Filter = [&HeaderFilters](llvm::StringRef Path) {
+      for (const auto &F : HeaderFilters)
+        if (F.match(Path))
+          return true;
+      return false;
+    };
+
     if (!HTMLReportPath.empty())
       writeHTML();
 
     auto &HS = getCompilerInstance().getPreprocessor().getHeaderSearchInfo();
     llvm::StringRef Path =
         SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
     assert(!Path.empty() && "Main file path not known?");
     llvm::StringRef Code = SM.getBufferData(SM.getMainFileID());
 
     auto Results =
         analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI, SM, HS);
     if (!Insert)
       Results.Missing.clear();
     if (!Remove)
       Results.Unused.clear();
     std::string Final = fixIncludes(Results, Code, getStyle(Path));
 
     if (Print.getNumOccurrences()) {
       switch (Print) {
       case PrintStyle::Changes:
-        for (const Include *I : Results.Unused)
+        for (const Include *I : Results.Unused) {
+          if (Filter(I->Resolved->tryGetRealPathName()))
+            continue;
           llvm::outs() << "- " << I->quote() << " @Line:" << I->Line << "\n";
-        for (const auto &I : Results.Missing)
+        }
+        for (const auto &I : Results.Missing) {
+          if (Filter(llvm::StringRef(I).trim("<>\"")))
+            continue;
           llvm::outs() << "+ " << I << "\n";
+        }
         break;
       case PrintStyle::Final:
         llvm::outs() << Final;
         break;
       }
     }
 
     if (Edit) {

 } // namespace include_cleaner
 } // namespace clang
 
 int main(int argc, const char **argv) {
   using namespace clang::include_cleaner;
 
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   auto OptionsParser =
       clang::tooling::CommonOptionsParser::create(argc, argv, IncludeCleaner);
   if (!OptionsParser) {
     llvm::errs() << toString(OptionsParser.takeError());
     return 1;
   }
 
   if (OptionsParser->getSourcePathList().size() != 1) {
     std::vector<cl::Option *> IncompatibleFlags = {&HTMLReportPath, &Print};
     for (const auto *Flag : IncompatibleFlags) {
       if (Flag->getNumOccurrences()) {
         llvm::errs() << "-" << Flag->ArgStr << " requires a single input file";
         return 1;
       }
     }
   }
   auto Factory = clang::tooling::newFrontendActionFactory<Action>();
   return clang::tooling::ClangTool(OptionsParser->getCompilations(),
                                    OptionsParser->getSourcePathList())
              .run(Factory.get()) ||
          Errors != 0;
 }