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

[Sam McCall via Phabricator via cfe-commits]

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