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