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