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