[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Haojian Wu via Phabricator via cfe-commits]

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

Thanks! Looks good from my side.

I'd wait to see whether @bkramer has more comments before commit it.



================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+  // used. These are used to rank results.
+  struct Signals {
+    Signals() {}
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > I think we can make a standalone class instead of making it a nested class of `SymbolInfo` because I don't see strong relationship between them. Maybe name it `FindingSignals` or `FindingInfo`.
> > The relationship between them is a strong scoping one: signals only make sense in the context of a particular SymbolInfo.
> > 
> > If it was a parallel top-level class, it needs a name that communicates this relationship, most likely SymbolSignals. I don't think that's significantly better than SymbolInfo::Signals.
> > 
> > (If I had my druthers, these would probably be Symbol and Symbol::Signals - the "info" is the main reason that SymbolInfo::Signals is noisy. But not worth the churn I think)
> > 
> You convinced me.  Please keep the comment of `Signals` updated.
> 
> > If I had my druthers, these would probably be Symbol and Symbol::Signals - the "info" is the main reason that SymbolInfo::Signals is noisy. But not worth the churn I think)
> 
> In the initial design, `SymbolInfo` merely represents a cpp symbol. But as more features developed, `SymbolInfo` might be not a good name at the moment. `Symbol` seems not a better name as LLVM already has many classes named `Symbol`. We can leave it here.
> 
> 
>  keep the comment of Signals updated.

You are missing this.


https://reviews.llvm.org/D30210