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