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

[Haojian Wu via Phabricator via cfe-commits]

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;
----------------
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.  


================
Comment at: include-fixer/SymbolIndexManager.cpp:104
+    for (const auto &SymAndSig : Symbols) {
+      const SymbolInfo Symbol = SymAndSig.Symbol;
       // Match the identifier name without qualifier.
----------------
I think you miss `&` here.


================
Comment at: include-fixer/find-all-symbols/FindAllMacros.cpp:37
                                  const MacroDirective *MD) {
-  SourceLocation Loc = SM->getExpansionLoc(MacroNameTok.getLocation());
-  std::string FilePath = getIncludePath(*SM, Loc, Collector);
-  if (FilePath.empty()) return;
+  if (auto Symbol = CreateMacroSymbol(MacroNameTok, MD->getMacroInfo())) {
+    FileSymbols[*Symbol].Seen++;
----------------
Nit: No `{}` for single statement in `if`. The same below.


================
Comment at: include-fixer/find-all-symbols/FindAllMacros.h:39
+
+  void Ifdef(SourceLocation Loc, const Token &MacroNameTok,
+             const MacroDefinition &MD) override;
----------------
We are missing tests for these macro usages.


================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.h:52
+  std::string Filename;
+  // Findings for the current source file, flushed on EndSourceFileAction.
+  SymbolInfo::SignalMap FileSymbols;
----------------
s/`on EndSourceFileAction`/`onEndOfTranslationUnit`.


================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+  // used. These are used to rank results.
+  struct Signals {
+    Signals() {}
----------------
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`.


================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:72
              int LineNumber, const std::vector<Context> &Contexts,
-             unsigned NumOccurrences = 0);
+             unsigned NumOccurrences = 0, unsigned NumUses = 0);
 
----------------
We don't need this method since we have `SymbolAndSignals`?


================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:101
 private:
-  friend struct llvm::yaml::MappingTraits<SymbolInfo>;
+  friend struct llvm::yaml::MappingTraits<struct SymbolAndSignals>;
 
----------------
I'd put this statement inside `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;
----------------
Not much better idea on names, how about `SymbolFinding`?

```
struct SymbolFinding {
   SymbolInfo Symbol;
   FindingInfo Finding;
};
```


https://reviews.llvm.org/D30210