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