[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.

thanks!



================
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)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139013