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