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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

A tool like this will be a useful addition!

I think the big high-level questions are:

- UI: a REPL seems like a good model, but we need a more user-friendly way to read commands
- Testing: need a plan for either a) testing the commands or b) keeping the commands simple/readable enough that we can get away without testing them.

I suspect a good design links these questions together (e.g. by establishing a simple pattern for reading/printing to keep the eval part simple, and read/print is most of the UI).

I'm not sure I agree this should be deferred until later.



================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:75
 add_subdirectory(global-symbol-builder)
+add_subdirectory(dexplorer)
----------------
if this is coupled to dex, and dex has its own directory, this should be a subdirectory I think


================
Comment at: clang-tools-extra/clangd/dexplorer/Dexplorer.cpp:27
+
+llvm::cl::opt<std::string> YAMLSymbolCollection(
+    "symbol-collection-file",
----------------
please avoid mentioning YAML, as we now have multiple formats.
"index file"?


================
Comment at: clang-tools-extra/clangd/dexplorer/Dexplorer.cpp:41
+// library for parsing flags and arguments.
+// FIXME(kbobyrev): Ideas for commands:
+// * fuzzy find symbol given a set of properties
----------------
find references


================
Comment at: clang-tools-extra/clangd/dexplorer/Dexplorer.cpp:62
+
+  llvm::outs() << "Scopes (comma-separated list):\n";
+  if (llvm::Optional<std::string> Line = LE.readLine()) {
----------------
Agree with the concerns about the usability of this model, a command-like model would be nicer.
If we want to start with something simple without worrying too much about complex inputs, maybe just accept queries one-per-line and don't allow the other options to be set yet?

Otherwise it seems like all of this is likely to need to be replaced in a backwards-incompatible way...


================
Comment at: clang-tools-extra/clangd/dexplorer/Dexplorer.cpp:122
+               << " ms.\n";
+  // FIXME(kbobyrev): Allow specifying which fields the user wants to see: e.g.
+  // origin of the symbol (CanonicalDeclaration path), #References, etc.
----------------
I'm not sure this will actually be a good UX.
Maybe just make sure you always include the symbol ID so the user can get details?


================
Comment at: clang-tools-extra/clangd/dexplorer/Dexplorer.cpp:147
+
+  // FIXME(kbobyrev): Wrap time measurements into something like
+  // measureTime(Function, Arguments...).
----------------
kbobyrev wrote:
> ilya-biryukov wrote:
> > +1 to this FIXME.
> > 
> > Something like:
> > ```
> > template <class Func>
> > auto reportTime(StringRef Name, Func F) -> decltype(F()) {
> >   auto Result = F();
> >   llvm::outs() << Name << " took " << ...
> >   return Result;
> > } 
> > ```
> > 
> > The code calling this API would be quite readable:
> > ```
> > auto Index = reportTime("Build stage", []() { 
> >   return buildStaticIndex(...);
> > });
> > ```
> Ah, this looks really clean! I had few ideas of how to do that, but I couldn't converge to a reasonable solution which wouldn't look like abusing C++ to me :) Thanks!
+1 - could you add this in this patch? would improve readability already


https://reviews.llvm.org/D51628