[PATCH] D139013: [include-cleaner] clang-include-cleaner can print/apply edits

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks!



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:65
+                        llvm::ArrayRef<SymbolReference> MacroRefs,
+                        const Includes &, const PragmaIncludes *PI,
+                        const SourceManager &SM, HeaderSearch &HS);
----------------
nit: name the `Includes` parameter. preferably `MainFileIncludes` (or `ProvidedIncludes`)?


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:77
+             bool Satisfied = false;
+             for (const Header &H : Providers) {
+               if (H.kind() == Header::Physical && H.physical() == MainFile)
----------------
nit: maybe leave a comment here for skipping `Header`s we've seen before. there's a quite good chance that we'll see same provider showing up multiple times, skipping processing might be helpful (later on we'll probably need to cache the analysis results for diagnostics purposes).


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:111
+    if (auto Edit = HI.insert(Insert.trim("<>\""), Insert.starts_with("<"))) {
+      if (PendingInsert && Edit->getOffset() == PendingInsert->getOffset()) {
+        PendingInsert = tooling::Replacement(
----------------
this logic makes me feel a little bit uneasy, as we're relying on alphabetical sorting of `Results.Missing`, which isn't just the filenames but also contains `<` or `"` at the beginning.
clang-format has weird include categories but I think it never mixes `"` includes with `<` includes. so we're probably safe here.
but it might still be safer to just keep a map of offsets to edits, up to you. (having a comment at the very least would be nice)


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:148
+)cpp";
+  Inputs.ExtraFiles["a.h"] = R"cpp(
+    #pragma once
----------------
nit: you can call guard(Code) here and elsewhere


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139013