sammccall 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;
----------------
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.
================
Comment at: include-fixer/find-all-symbols/FindAllMacros.h:39
+
+ void Ifdef(SourceLocation Loc, const Token &MacroNameTok,
+ const MacroDefinition &MD) override;
----------------
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.
================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+ // used. These are used to rank results.
+ struct Signals {
+ Signals() {}
----------------
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)
================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:101
private:
- friend struct llvm::yaml::MappingTraits<SymbolInfo>;
+ friend struct llvm::yaml::MappingTraits<struct SymbolAndSignals>;
----------------
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.
================
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;
----------------
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 :)
https://reviews.llvm.org/D30210
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
