[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/lib/Analysis.cpp:77
+             bool Satisfied = false;
+             for (const Header &H : Providers) {
+               if (H.kind() == Header::Physical && H.physical() == MainFile)
----------------
kadircet wrote:
> sammccall wrote:
> > 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.
> > We still need to process each header to compute Satisfied
> 
> but we need to do this only once per unique `Providers` across all the callbacks (e.g. we'll get `Foo.h` as a provider for every reference of `Foo` and `createFoo` in the main file). From header analysis point of view, i don't think we need to treat each of these Providers specially based on the nature of the reference (neither the symbol nor the reference location/type).
> 
> > 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).
> 
> Yeah I guess you're right. I was treating the inner-loop as going over all the includes in the main file, but in practice it's just a bunch of lookups into a hashtable and operating over a single include.
> 
> > 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.
> 
> no need. agreed that it is unlikely to make a difference. and if it would, it's probably better to write this in a way that'll de-duplicate Providers across references  with some bucketing over reftypes and process all of them once.
> but we need to do this only once per unique Providers across all the callbacks (e.g. we'll get Foo.h as a provider for every reference of Foo and createFoo in the main file)

By "process" I mean we need to at least look up into the cache.
That's pretty trivial, but we're not doing much more at the moment.

> but in practice it's just a bunch of lookups into a hashtable

in fact just one lookup!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139013