[PATCH] D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration

[Sam McCall via Phabricator via cfe-commits]

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