sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Really just details now. ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:56 +// llvm::formatv string pattern for pretty-printing symbols. +static const auto OutputFormat = "{0,-4} | {1,-40} | {2,-25}\n"; + ---------------- (I don't think you want to share this between fuzzyfind and lookup, see below) ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:62 + Request.Query = UnqualifiedName; + std::vector<Symbol> Symbols; + llvm::outs() << '\n'; ---------------- unused ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:63 + std::vector<Symbol> Symbols; + llvm::outs() << '\n'; + // FIXME(kbobyrev): Print symbol final scores to see the distribution. ---------------- why? (and below) If you want all commands to be separated by a blank line, the dispatcher can do it ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:64 + llvm::outs() << '\n'; + // FIXME(kbobyrev): Print symbol final scores to see the distribution. + llvm::outs() << llvm::formatv(OutputFormat, "Rank", "Symbol ID", ---------------- you have this comment twice ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:90 +void lookup(StringRef USR, const SymbolIndex &Index) { + llvm::DenseSet<clang::clangd::SymbolID> IDs{clang::clangd::SymbolID{USR}}; + clang::clangd::LookupRequest Request{IDs}; ---------------- note that you're looking up one ID so you'll always get 0 or 1 result ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:95 + "Symbol Name"); + size_t Rank = 0; + Index.lookup(Request, [&](const Symbol &Sym) { ---------------- Rank is never meaningful for lookup, especially when only looking up one id ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:97 + Index.lookup(Request, [&](const Symbol &Sym) { + llvm::outs() << llvm::formatv(OutputFormat, Rank++, Sym.ID.str(), Sym.Name); + }); ---------------- this isn't enough detail to be really useful. I'd suggest dumping the whole symbol as YAML, or printing 'not found' if no result. ================ Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:125 +// * print out tokens with least dense posting lists +void dispatchRequest(const std::unique_ptr<SymbolIndex> &Index, + StringRef Request) { ---------------- sammccall wrote: > ilya-biryukov wrote: > > This function does too many things: > > - parses the input. > > - dispatches on different kinds of requests. > > - handles user input errors. > > - actually runs the commands. > > > > This turns out somewhat unreadable at the end, we might want to separate > > those into multiple functions. Will need a bit of time to think on how to > > actually do it best. Will come back after I have concrete suggestions. > For now can we keep it simple: > - this function can split the command from the rest, and pass the rest of > the args to `fuzzyFindRequest` etc > - if the command is unknown, the dispatcher reports the error > - if the command is known, the command handler reports input errors (this > could be cleaner/more declarative, but we don't want to build a big framework > in this patch) > - this function could do the timing - the request construction is negligible > and that keeps the command handlers focused on their job this is not done (dispatchRequest is still doing validation for each command) ================ Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:130 + if (Arguments.empty()) { + llvm::outs() << "ERROR: Request can not be empty.\n"; + helpRequest(); ---------------- sammccall wrote: > REPLs don't usually make this an error, just return this is not done ================ Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:180 + + llvm::LineEditor LE("dexplorer"); + ---------------- dexp? https://reviews.llvm.org/D51628 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits