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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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);
----------------
kadircet wrote:
> nit: name the `Includes` parameter. preferably `MainFileIncludes` (or `ProvidedIncludes`)?
Added a name for consistency, but I don't think the name is actually useful here.

("MainFile" is inconsistent with the other parameters, and "Provided" is redundant for an input parameter)


================
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)
----------------
kadircet wrote:
> 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).
I think you're talking about a performance optimization using a cache?

We still need to process each header to compute `Satisfied`. So at most we're replacing a trivial comparison + 2 hashtable lookups (`Inc.match` and `Used.insert`) with one cache lookup. (The inner loop count is ~always <=1).

Happy with such a change if it improves a benchmark of course but my expectation is that it *wouldn't* (small constant factor on a fairly fast operation) so a FIXME seems premature here.


================
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(
----------------
kadircet wrote:
> 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)
TL;DR: replaced implementation with clang-format, tweaked signature

---

Good catch, this is completely wrong, I should capture all the edits, stable_sort them, and then fold together.

But this got me thinking that actually this doesn't give the correct relative order of inserted headers: they'll just be in whatever order I start with. I switched to using libFormat instead - I can't really understand from the implementation how it solves this problem, but it probably does, and if not it's the right place to fix it.

This had some knock-on effects on the API: no more messing with MemoryBuffer etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139013