hokein added inline comments.
================
Comment at: include-fixer/InMemorySymbolIndex.h:27
- std::vector<clang::find_all_symbols::SymbolInfo>
+ std::vector<clang::find_all_symbols::SymbolAndSignals>
search(llvm::StringRef Identifier) override;
----------------
sammccall wrote:
> hokein wrote:
> > There are many places using
> > `std::vector<clang::find_all_symbols::SymbolAndSignals>`. Maybe we can use
> > a type alias for it, so that we can type less.
> I guess? It's the namespaces that are the problem (vector<SymbolAndSignals>
> is fine) and most of the namespace noise wouldn't go away here.
>
> is `clang::find_all_symbols::SymbolsSignalsList` better enough to obscure
> what the actual type is? It's 45 chars vs 54.
>
> IMO it's not worth it here, though
> `clang::find_all_symbols::SymbolInfo::SignalMap` vs
> `std::map<clang::find_all_symbols::SymbolInfo,
> clang::find_all_symbols::SymbolInfo::Signals>` is.
If we put the type alias under `clang::include_fixer` namespace, it will
shorten the name more. Agree it is not worth the effect as the full name only
happens in headers.
We could save a few characters by getting rid of `clang` because we are always
in `clang` namespace. So `std::vector<find_all_symbols::SymbolAndSignals>`
should work, this looks slightly better. :)
================
Comment at: include-fixer/find-all-symbols/FindAllMacros.cpp:38
+ if (auto Symbol = CreateMacroSymbol(MacroNameTok, MD->getMacroInfo()))
+ FileSymbols[*Symbol].Seen++;
+}
----------------
code style: use prefix `++`. The same below.
================
Comment at: include-fixer/find-all-symbols/FindAllMacros.h:39
+
+ void Ifdef(SourceLocation Loc, const Token &MacroNameTok,
+ const MacroDefinition &MD) override;
----------------
sammccall wrote:
> hokein wrote:
> > We are missing tests for these macro usages.
> These are covered by FindAllSymbolsTests, which (despite the name) tests the
> whole FindAllSymbolsAction.
>
> Specifically, MacroTest and MacroTestWithIWYU cover these.
Acked. You combined them in current tests (I originally thought there should be
some separate tests for these).
================
Comment at: include-fixer/find-all-symbols/SymbolInfo.cpp:79
const std::vector<Context> &Contexts,
- unsigned NumOccurrences)
+ unsigned NumOccurrences, unsigned NumUses)
: Name(Name), Type(Type), FilePath(FilePath), Contexts(Contexts),
----------------
You forgot to update this?
================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+ // used. These are used to rank results.
+ struct Signals {
+ Signals() {}
----------------
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.
================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:101
private:
- friend struct llvm::yaml::MappingTraits<SymbolInfo>;
+ friend struct llvm::yaml::MappingTraits<struct SymbolAndSignals>;
----------------
sammccall wrote:
> hokein wrote:
> > I'd put this statement inside `SymbolAndSignals`.
> That won't compile: it's the members of SymbolInfo that are private, not the
> members of SymbolAndSignals.
Acked. Thanks for explanations. Sorry for misleading.
================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:129
- /// \brief The number of times this symbol was found during an indexing
- /// run. Populated by the reducer and used to rank results.
- unsigned NumOccurrences;
+struct SymbolAndSignals {
+ SymbolInfo Symbol;
----------------
sammccall wrote:
> hokein wrote:
> > Not much better idea on names, how about `SymbolFinding`?
> >
> > ```
> > struct SymbolFinding {
> > SymbolInfo Symbol;
> > FindingInfo Finding;
> > };
> > ```
> I don't think SymbolFinding is better:
> - it can be misinterpreted as finding *for* a signal, not findings *and* a
> signal. I think the And is important
> - "finding" is vague while "signal" is more specific. I changed this from
> finding -> signal already based on a discussion with Ben, if you do want to
> change this we should sync up offline :)
Fair enough. I don't have strong opinion about the name.
https://reviews.llvm.org/D30210
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
