ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+ {"configurationChangeProvider", true},
{"renameProvider", true},
----------------
simark wrote:
> ilya-biryukov wrote:
> > 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.
> I've removed it for now, we can add it later. We should think about how to
> express what we support. It's not enough to say we support the
> `didChangeConfiguration` notification, we should also express what element of
> configuration we support (the location of the compile commands directory, in
> this case).
SG. Let's not add extra stuff not in the LSP if the clients can live without it.
================
Comment at: clangd/ClangdLSPServer.cpp:302
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
----------------
simark wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > 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.
> > > FIXME looks fine for now, but make sure to test this code does really
> > > work for your use-case.
> >
> > So far I have just tested with some small hello-world projects, but I'll
> > take the time to test with some bigger project (gdb).
> > FIXME looks fine for now, but make sure to test this code does really work
> > for your use-case.
>
> I've tested many scenarios with a project of good size. It works as
> expected, but I've hit the assert in `ClangdServer::forceReparse` maybe
> twice. I don't know how to reproduce it. I think we can go ahead with the
> patch and maybe the problem will become clearer with time. At least even if
> this feature is not completely stable, it won't affect you if you don't use
> it.
What was the assertion? "forceReparse called for non-added document"?
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
