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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:23
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/YAMLTraits.h"
+#include <cstdlib>
----------------
why?


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:56
+
+template <class Function, class TimeUnit = std::chrono::milliseconds>
+void reportTime(StringRef Name, Function F) {
----------------
ilya-biryukov wrote:
> Why do we want `TimeUnit`?
> It adds complexity, if we want to make large values more readable we have other alternatives:
> - printing adaptive units, e.g. "when it's larger than 5000ms, start printing seconds", "when it's larger than 5minutes, start printing minutes"
> - provide separators (`500000000ms`  is pretty much unreadable, `500 000 000ms` is much better)
> 
+1 just pick ms or us for now

You can use `formatv({0:ms}, Duration)` to print neatly, no need for the traits.


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:79
+
+static const std::string HelpMessage = R"(
+DExplorer commands:
----------------
nit: this starts with a newline


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:82
+
+> fuzzy-find Name
+
----------------
nit: just "find" for convenience?


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:87
+
+> lookup-id SymbolID
+
----------------
examples use `lookup-id`, code says `lookup`.
(I prefer `lookup` FWIW)


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:90
+Retrieves symbol names given USR.
+
+> help
----------------
(Not sure the "help" doc or "press ctrl-d to exit" are particularly useful)


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:100
+
+void lookupRequest(const std::unique_ptr<SymbolIndex> &Index,
+                   clang::clangd::LookupRequest &Request) {
----------------
SymbolIndex&, you don't need the unique_ptr


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:100
+
+void lookupRequest(const std::unique_ptr<SymbolIndex> &Index,
+                   clang::clangd::LookupRequest &Request) {
----------------
sammccall wrote:
> SymbolIndex&, you don't need the unique_ptr
nit: lookup (the "request" is the arg)


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:103
+  std::vector<Symbol> Symbols;
+  Index->lookup(Request, [&](const Symbol &S) { Symbols.push_back(S); });
+  // FIXME(kbobyrev): Print symbol final scores to see the distribution.
----------------
why not print them on-the-fly?
(this isn't safe, the data backing the symbols may not live long enough)


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:106
+  llvm::outs() << "\nRetrieved Symbols:\n";
+  llvm::outs() << "Rank. Symbol Name | Symbol ID\n";
+  for (size_t Rank = 0; Rank < Symbols.size(); ++Rank)
----------------
please be consistent in using . or | as separator


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:106
+  llvm::outs() << "\nRetrieved Symbols:\n";
+  llvm::outs() << "Rank. Symbol Name | Symbol ID\n";
+  for (size_t Rank = 0; Rank < Symbols.size(); ++Rank)
----------------
sammccall wrote:
> please be consistent in using . or | as separator
I'd suggest putting the ID first, as it's fixed-width so the columns will actually be columns


================
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) {
----------------
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


================
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();
----------------
REPLs don't usually make this an error, just return


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:157
+  } else {
+    helpRequest();
+  }
----------------
as above, resist the temptation to dump the help on every opportunity...
just "Unknown command. Try 'help'." or so is more friendly I think


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:167
+
+  // FIXME(kbobyrev): Wrap time measurements into something like
+  // measureTime(Function, Arguments...).
----------------
fixme is done


================
Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:176
+  if (!Index) {
+    llvm::outs() << "ERROR: Please provide a valid YAML symbol collection.\n";
+    return -1;
----------------
still says YAML


https://reviews.llvm.org/D51628