[PATCH] D44882: [clangd] Implementation of workspace/symbol request

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.


================
Comment at: clangd/SourceCode.cpp:110
+
+llvm::Optional<Location> offsetRangeToLocation(SourceManager &SourceMgr,
+                                               StringRef File,
----------------
MaskRay wrote:
> May I ask a question about the conversion between SourceLocation and LSP location? When the document is slightly out of sync with the indexed version, what will be returned?
I forgot to cover the case of the unsaved files, which are indexed in memory. It that what you mean? I'll update the patch to address by using the DraftStore when available. There is also the case where the file is not opened but outdated on disk. I don't think we can do much about it but make sure it doesn't crash :) At worst, the location might be off, and navigation will be momentarily affected, until the index can be updated with the file change. This is what I've noticed in other IDEs as well.


================
Comment at: clangd/tool/ClangdMain.cpp:105
 
+static llvm::cl::opt<int> LimitWorkspaceSymbolResult(
+    "workspace-symbol-limit",
----------------
MaskRay wrote:
> malaperle wrote:
> > sammccall wrote:
> > > the -completion-limit was mostly to control rollout, I'm not sure this needs to be a flag. If it does, can we make it the same flag as completions (and call it -limit or so?)
> > I think it's quite similar to "completions", when you type just one letter for example, you can get a lot of results and a lot of JSON output. So it feels like the flag could apply to both completions and workspace symbols. How about -limit-resuts? I think just -limit might be a bit too general as we might want to limit other things.
> Can these options be set by LSP initialization options?
They could. Are you say they *should*? We could add it in DidChangeConfigurationParams/ClangdConfigurationParamsChange (workspace/didChangeConfiguration) if we need to. I haven't tried or needed to add it on the client side though. It's not 100% clear what should go in workspace/didChangeConfiguration but I think it should probably things that the user would change more often at run-time. I'm not sure how frequent this "limit" will be changed by the user.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44882