sammccall added a comment.
Seems fair enough to do something about this, though it's a bit sad we're doing
it just because VSCode has bad UI.
A couple of open design questions:
1. In the future it may make sense to have other documentLinks too. (e.g.
imagine a comment `Configures the Frobnicator` - we should linkify Frobnicator
to point to a class, once documentLink supports target ranges). So we should
decide if this feature is "disable documentLink" or "don't include included
headers in documentLink". This affects e.g. naming of the flag, which is
awkward to change later.
2. Do we really want to add command-line flags for this or does it rather
belong as a config option? (In which case we'd want to change the behavior of
the method, not turn the method itself on/off, to allow the config option to
vary across files in the usual way).
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:639
Result.getObject("capabilities")->insert({"foldingRangeProvider", true});
+ if (Opts.IncludesAsLinks)
+ Result.getObject("capabilities")
----------------
this is worth a comment:
Currently we only provide documentLink for #included headers. If that's turned
off, let clients avoid sending the request altogether.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1513
MsgHandler->bind("textDocument/foldingRange",
&ClangdLSPServer::onFoldingRange);
+ if (Opts.IncludesAsLinks)
+ MsgHandler->bind("textDocument/documentLink",
&ClangdLSPServer::onDocumentLink);
----------------
It seems simpler for debugging and tests to leave the method bound regardless,
and only associate the *capability* with the option.
This is what we usually do, though foldingRange wasn't done this way for some
reason. (Maybe because we know it's *crashy* so we're being extra cautious). In
any case, no need for it in this case.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:55
std::function<void()> MemoryCleanup = nullptr;
+ /// If set, include files in #include directives are exposed to the clients
+ /// as documentLinks. If disabled include files can still be opened with
the
----------------
this belongs below in the list of per-feature options.
================
Comment at: clang-tools-extra/clangd/test/xrefs.test:4
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"extern
int x;\nint x = 0;\nint y = x;"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include
<stdint.h>\nextern int x;\nint x = 0;\nint y = x;"}}}
---
----------------
Having tests depend on external headers is a bit of a pain downstream - it
means they need to run in an environment where the headers are available.
We do this by necessity in `document-link.test` - maybe we should move this
test here?
(Personally I'm comfortable with this functionality only being covered by
unit-tests, as it is now though).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93763/new/
https://reviews.llvm.org/D93763
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
