sammccall added a comment.
Thanks, looks nice!
It occurred to me we could compute (but not resolve) the ranges cheaply to
speed up the UI. We don't need to do this now.
Only real thing to do is add a gunit test.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1204
+void ClangdLSPServer::onDocumentLink(
+ const DocumentLinkParams &Params,
----------------
Eagerly resolving everything means that this request is going to block on the
preamble/AST being built. This isn't terrible (clients should be sending their
initial requests in parallel) but does mean a delay before the links show up.
(And in updating if we insert an include and want to ctrl-click it, because we
just invalidated the preamble).
There are a few ways we could improve this:
a) find the links using a quick pass (simple string matching or raw-lexer
based), then resolve results from the MainFileIncludes structure (blocking on
the AST at that point)
b) use a quick pass to serve results in the first place, e.g. a
PreprocessorOnlyAction with SingleFileParseMode (doesn't descend into headers).
c) a combination of these.
I don't particularly think we need to do these at this point, but may be worth
a comment.
================
Comment at: clang-tools-extra/clangd/Protocol.h:1263
+///
+/// TODO(forster): For now only file URIs are supported.
+struct DocumentLink {
----------------
I don't think this TODO is needed - there's nothing to do unless we decide we
want to emit other types of links.
(Which seems fairly unlikely: my understanding is that e.g. hyperlinks in
comments should be resolved by editors generically rather than by language
servers)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:181
+ if (!Inc.Resolved.empty()) {
+ Result.emplace_back(DocumentLink(
+ {Inc.R, URIForFile::canonicalize(Inc.Resolved, *MainFilePath)}));
----------------
nit `push_back(DocumentLink{...})`
================
Comment at: clang-tools-extra/clangd/test/document-link.test:1
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
----------------
Generally we smoke-test features in a lit test such as this one, and then do
fine-grained testing as gunit tests (e.g. unittests/XRefTests.cpp). It's good
to test end-to-end, but it's too hard to maintain lit tests for all cases as
features get extended.
For this feature there's not much difference between the two, but you could
drop one of the includes here and cover one more case in the unit tests:
```
#include "foo.h"
int end_of_preamble = 0;
#include "not_part_of_preamble.h"
```
(The non-preamble includes get into the data structures you're querying via a
different path).
The unit tests are generally easier to set up:
- you can use Annotations to write code with marked regions, and get the
coordinates + unmarked code
- you can use TestTU to add extra files "foo.h" to the VFS, and produce a
ParsedAST
- then just assert that the results match testPath("foo.h") +
annotations.points()[0] etc.
`TEST(LocateSymbol, WithIndex)` in `XRefsTests.cpp` is a reasonable example.
================
Comment at: clang-tools-extra/clangd/test/document-link.test:21
+# CHECK-NEXT: },
+# CHECK-NEXT: "target": "file://{{.*}}/iostream"
+# CHECK-NEXT: },
----------------
MForster wrote:
> Originally I tried to add a header file to the setup with a second didOpen
> request, but I didn't get this to work. Would I need to set this up like
> `background-index.test`, or is there a simpler way?
>
> Anyway, I think the regular expression is probably good enough for the
> purpose of this test.
> Originally I tried to add a header file to the setup with a second didOpen
> request, but I didn't get this to work
Clangd's model is that the *current* file is always via LSP (didOpen), and all
other files are read from disk. The setup would be slightly simpler than
background-index.test because you don't need a compile command for the other
file, but still a bit awkward.
I think the regex is OK, but would prefer to change to `stdint.h`/`stddef.h`
(not the C++ versions). This is because they're builtin headers in clang, so
even if we don't find a standard library they'll still exist. (clangd tests do
formally depend on the builtins but not a stdlib, I believe).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70872/new/
https://reviews.llvm.org/D70872
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
