YAML serialization was used in the Proof of Concept for simplicity.
This patch replaces implements Protobuf (de) serialization of almost all
types that need to be transferred over the protocol.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
It's tedious, but we really should have tests for this now.
clang-tools-extra/clangd/index/remote/Index.proto | ||
---|---|---|
68–113 | somewhere in this file there should be a hint that semantics of all fields matches those in the structs in Index/ | |
70 | sigh, I wish we'd managed to get rid of this struct by now :-( |
clang-tools-extra/clangd/unittests/TestTU.h | ||
---|---|---|
1 | This header provides a widely used abstraction and has a clear scope, please don't dump things here. | |
94 | I don't think the API of this class has been thought through well enough to share. Why do you need it? Can't you just use TestTU::headerSymbols() (if you need Refs and such as well, just copy its implementation - it's 2 lines long) |
Just more stuff about simplifying the tests :-(
This is really just about converting between a struct and a proto, if we have to add indexing-related fixtures to shared libraries something seems to have gone a bit off the rails.
In the worst case we could just create a few symbols with fields populated by hand, it's probably easier to see what parts of the code are being covered by the test...
clang-tools-extra/clangd/unittests/TestTU.cpp | ||
---|---|---|
128 | can you structure the tests to avoid doing this (untested) merge? Why do we need the example symbols split over multiple files at all? | |
clang-tools-extra/clangd/unittests/TestTU.h | ||
73 | please don't add these for a single test, we can do this inline in the test unless it becomes a common thing | |
clang-tools-extra/clangd/unittests/remote/CMakeLists.txt | ||
2 | Can we conditionally include these tests into the main ClangdTests, instead of setting up another set of targets? | |
18 | I don't think including the same source files in multiple targets is a great path to go down. | |
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp | ||
20 | Using inheritance to define test *cases* is a bit of a trap, and not particularly idiomatic. | |
62 | similarly i'd consider just inlining the stringsaver/arena into the tests, seems easier to read | |
66 | if you want to define the symbols indirectly like this, you probably also want to assert that there are more than 5 or something, to guard against the test fixture being broken |
Address some comments.
clang-tools-extra/clangd/unittests/remote/CMakeLists.txt | ||
---|---|---|
2 | I can do that, although I saw that XPC has their own set of tests if I am correct, so I thought this should be OK. Conditionally including tests into the main ClangdTests is not super easy because... CMake stuff. But I. can look into that. |
Move remote tests into all Clangd tests
clang-tools-extra/clangd/unittests/remote/CMakeLists.txt | ||
---|---|---|
2 | Actually, nevermind, it looks pretty straightforward. |
Thanks! Sorry about the hassle with the tests.
clang-tools-extra/clangd/unittests/remote/CMakeLists.txt | ||
---|---|---|
2 | Yeah we have to pay the cost of *some* cmake stuff either way, this looks like a bit less work. XPC... yeah, I wish there was less CMake stuff going on there. But in practice, the majority of maintenance in the two dirs has been disjoint sets of people, so it has its advantages to... |
No worries, I think it looks cleaner and simpler which is important.
Thanks for the review!
sigh, I wish we'd managed to get rid of this struct by now :-(
Maybe we can still do it before the 11 release