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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
