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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the contributions!



================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:244
 
-  const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("decl");
-  assert(ND && "Matched declaration must be a NamedDecl!");
+  auto report = &SymbolReporter::reportSymbol;
+  const NamedDecl *ND;
----------------
The way seems too tricky to me. I'd use a flag variable like `isUsedDecl` to distinguish and handle `use` and `decl` cases.


================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:250
+  } else {
+    assert(!"Must match a NamedDecl!");
+  }
----------------
Is the preceding `!` intended?


================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:83
+  /// run. Populated by the reducer and used to rank results.
+  mutable unsigned NumOccurrences;
+  /// \brief The number of times this symbol was used during an indexing run.
----------------
A blank between class members.


================
Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:551
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_FALSE(hasUse(Symbol));  // Not yet implemented.
 
----------------
The same.


================
Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:48
   bool hasSymbol(const SymbolInfo &Symbol) const {
-    for (const auto &S : Symbols) {
-      if (S == Symbol)
-        return true;
-    }
-    return false;
+    return std::find(Symbols.begin(), Symbols.end(), Symbol) != Symbols.end();
+  }
----------------
nit: You can use `llvm::is_contained` here.


================
Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:52
+  bool hasUse(const SymbolInfo &Symbol) const {
+    return std::find(Used.begin(), Used.end(), Symbol) != Used.end();
   }
----------------
The same.


================
Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:65
   }
+  bool hasUse(const SymbolInfo &Symbol) {
+    return Reporter.hasUse(Symbol);
----------------
nit: a blank line between methods.


================
Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:526
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_FALSE(hasUse(Symbol));  // Not yet implemented.
 
----------------
I'd put a `FIXME` comment here.


https://reviews.llvm.org/D30210