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