ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+ {"configurationChangeProvider", true},
{"renameProvider", true},
----------------
simark wrote:
> Nebiroth wrote:
> > simark wrote:
> > > I find `configurationChangeProvider` a bit weird. It makes it sound like
> > > clangd can provide configuration changes. In reality, it can accept
> > > configuration changes. So I think this should be named something else.
> > Agreed, perhaps configurationChangeManager would be more appropriate then?
> I'm thinking of removing it for the time being. Since it's not defined in
> the protocol what types of configuration changes exist (it's specific to each
> language server), it not very useful to simply advertise that we support
> configuration changes. We would need to advertise that we support
> compilation database changes in particular. I think this can be done later.
`"configurationChangeProvider"` is not in the LSP, right?
There's `experimental` field in the specification, let's put it under that
field if you want to advertise that clangd supports this spec to your clients.
================
Comment at: clangd/ClangdLSPServer.cpp:302
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
----------------
simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > ilya-biryukov wrote:
> > > > Are you planning to to address this FIXME before checking the code in?
> > > Following what you said here:
> > >
> > > https://reviews.llvm.org/D39571?id=124024#inline-359345
> > >
> > > I have not really looked into what was wrong with the test, and what is
> > > missing in the infrastructure to make it work. But I assumed that the
> > > situation did not change since then. Can you enlighten me on what the
> > > problem was, and what is missing?
> > We usually write unittests for that kind of thing, since they allow to plug
> > an in-memory filesystem, but we only test `ClangdServer` (examples are in
> > `unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not allow to
> > plug in a virtual filesystem (vfs). Even if we add vfs, it's still hard to
> > unit-test because we'll have to match the json input/output directly.
> >
> > This leaves us with an option of a lit test that runs `clangd` directly,
> > similar to tests in `test/clangd`.
> > The lit test would need to create a temporary directory, create proper
> > `compile_commands.json` there, then send the LSP commands with the path to
> > the test to clangd.
> > One major complication is that in LSP we have to specify the size of each
> > message, but in our case the size would change depending on created temp
> > path. It means we'll have to patch the test input to setup proper paths and
> > message sizes.
> > If we choose to go down this path,
> > `clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar setup
> > (create temp-dir, patch up some configuration files to point into the temp
> > directory, etc) and could be used as a starting point.
> >
> > It's not impossible to write that test, it's just a bit involved. Having a
> > test would be nice, though, to ensure we don't break this method while
> > doing other things. Especially given that this functionality is not used
> > anywhere in clangd.
> > We usually write unittests for that kind of thing, since they allow to plug
> > an in-memory filesystem, but we only test ClangdServer (examples are in
> > unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not allow to plug
> > in a virtual filesystem (vfs). Even if we add vfs, it's still hard to
> > unit-test because we'll have to match the json input/output directly.
>
> What do you mean by "we'll have to match the json input/output directly"?
> That we'll have to match the complete JSON output textually? Couldn't the
> test parse the JSON into some data structures, then we could assert specific
> things, like that this particular field is present and contains a certain
> substring, for example?
>
> > This leaves us with an option of a lit test that runs clangd directly,
> > similar to tests in test/clangd.
> > The lit test would need to create a temporary directory, create proper
> > compile_commands.json there, then send the LSP commands with the path to
> > the test to clangd.
> > One major complication is that in LSP we have to specify the size of each
> > message, but in our case the size would change depending on created temp
> > path. It means we'll have to patch the test input to setup proper paths and
> > message sizes.
> > If we choose to go down this path,
> > clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar setup
> > (create temp-dir, patch up some configuration files to point into the temp
> > directory, etc) and could be used as a starting point.
>
> Ok, I see the complication with the Content-Length. I am not familiar with
> lit yet, so I don't know what it is capable of. But being able to craft and
> send arbitrary LSP messages would certainly be helpful in the future for all
> kinds of black box test, so having a framework that allows to do this would
> be helpful, I think. I'm not familiar enough with the ecosystem to do this
> right now, but I'll keep it in mind.
>
> One question about this particular test. Would there be some race condition
> here? If we do:
>
> 1. Start clangd with compile_commands.json #1
> 2. Ask for the definition of a function, expecting a result
> 3. Change the configuration to compile_commands.json #2
> 4. Ask for the definition of the same function, expecting a different result
>
> Since clangd is multi-threaded and does work in the background, are we sure
> that we'll get the result we want in #4?
>
> > It's not impossible to write that test, it's just a bit involved. Having a
> > test would be nice, though, to ensure we don't break this method while
> > doing other things. Especially given that this functionality is not used
> > anywhere in clangd.
>
> I agree. For the time being, is it fine to leave the FIXME there? We can
> work on improving the test frameworks to get rid of it.
> What do you mean by "we'll have to match the json input/output directly"?
> That we'll have to match the complete JSON output textually? Couldn't the
> test parse the JSON into some data structures, then we could assert specific
> things, like that this particular field is present and contains a certain
> substring, for example?
The interface to interact with `ClangdLSPServer` is `JSONOutput`, which only
allows you to pass the output of requests to the stream at the moment. That
means not only parsing json, but also finding the individual responses in the
combined output.
> One question about this particular test. Would there be some race condition
> here? If we do:
Technically clangd can do everything in parallel, but we have a flag
`-run-synchronously` that will make it do all the work on the main thread and
we use that in the tests.
LLVM has lots of tests that do substring matches, there's a special tool,
called FileCheck, to make writing them simpler. See the test from my previous
message (specifically the `# CHECK: ` lines), they should be enough to get
started.
Lit itself is a set of python scripts that allow, among other things, to
specify directly in the test file which commands the test needs to run. Again,
see the example tests (specifically, `# RUN: clangd ....` lines).
> I agree. For the time being, is it fine to leave the FIXME there? We can work
> on improving the test frameworks to get rid of it.
FIXME looks fine for now, but make sure to test this code does really work for
your use-case.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D39571
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
