Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generally I think this is missing high-level documentation describing the translation that happens at each level - I had to refer back to the notes from our meeting to understand what the scheme was.
I think Marshalling.h is probably a good place to document how this works. (Not exactly what prefixes get stripped by which function, but rather what the backing index contains, what goes over the wire, and what the clangd on the client side sees).
clang-tools-extra/clangd/index/remote/Client.cpp | ||
---|---|---|
107 | this should be a std::string - no need to rely on the caller to keep the value alive | |
clang-tools-extra/clangd/index/remote/Client.h | ||
21 | This is a bit low-level - it describes what is being done but not really why. As a caller of this function it doesn't really tell me what I should pass. Suggest something like "ProjectRoot is an absolute path on the local machine to the source tree described by the remote index. Paths returned by the index will be treated as relative to this directory." I'd also suggest calling this IndexRoot instead of ProjectRoot, because the latter is a fairly overloaded concept and they need not coincide. | |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp | ||
34 | mention somewhere that this is code used on the *client* side to do translation, and that RelativePath came from the server | |
38 | scheme is always "file", no need to make it a parameter? | |
41 | 20 is too small. Suggest 256 or so (SmallString is on the stack, so it's cheap) | |
43 | you can use URI::createFile which does less work and never fails | |
49 | this looks like a no-op cast | |
52 | Again this is a bit low level - "translates a URI from the server's backing index to a relative path suitable to send over the wire to the client"? | |
55 | uriToRelativePath for consistency? | |
61 | you're crashing here with an unhandled Expected. Add a test? | |
64 | static_cast is a pretty surprising/uncommon way to write this conversion.... | |
65 | need to handle the case where this returns false. Probably log and return ""? Again, we should have a test for this. | |
141–142 | this needs to do path translation in the case where the header is a URI | |
174 | and here | |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h | ||
46 | nit: Clangd -> clangd | |
59–60 | what does "serializes clangd" mean? | |
clang-tools-extra/clangd/index/remote/server/Server.cpp | ||
35 | nit: call this variable IndexRoot rather than IndexPrefixPath, for consistency? | |
69–70 | for hygiene, I think we should pass this in as a constructor parameter rather than accessing the flag directly. You probably also want to add a little validation: this needs to be an absolute path, and you may need to convert slashes to unix-style, too. | |
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp | ||
102 | this is testPath("unittest"), isn't it? I mean, forward vs backslashes are going to differ, but the current expression is going to produce C:\test/unittest which doesn't seem better. | |
186 | test name is backwards - TEST(RemoteMarshallingTest, URITranslation) ? |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp | ||
---|---|---|
39 | we should also bail out if RelativePath is an absolute path. | |
65 | we really should ensure the result has unix slashes. | |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h | ||
59–60 | The Ref marshaller isn't an appropriate place to put the detailed documentation on how path translation works. | |
60 | Somewhere we need to describe what the format of ProjectRoot is: trailing slash or not, unix slashes or native, etc. | |
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp | ||
100–101 | Is passing the empty string here actually valid? I think you probably want to pass testRoot() or some unixified equivalent | |
186 | I think this test is written in a way where it won't work on windows. | |
191 | this is not (lexically) a relative path, it's an absolute path. | |
196 | OK, this assertion does actually show that we're sending absolute paths over the wire and calling them relative. |
Store the progress so that I don't lose it. Still WIP.
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp | ||
---|---|---|
64 | Okay, sure! I just thought .str().str() looks really weird, but std::string is probably a good choice. | |
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp | ||
100–101 | Why testroot? That's what I'm stripping from URI body, the URI is unittest:///TestTU.h, hence I'm stripping from /TestTU.h, there is no /clangd-test there. Could you elaborate? | |
102 | I don't understand: std::string(testRoot()) + '/' is /clangd-test/ on unix and testPath("unittest") is /clangd-test/unittest. I understand the slashes argument, but could you please elaborate on the former? |
Convert prefix paths to native slash format for _serialization_ calls
(deserializations should use UNIX slashes).
This looks better to me but:
- i think we need to consider what happens when paths are not under the expected path, and handle that in some appropriate way
- I find it hard to see what the tests are actually testing - would be good to make them more explicit, using a few minimal helpers, and avoiding fancy abstractions like URI schemes
clang-tools-extra/clangd/index/remote/Index.proto | ||
---|---|---|
102–105 | storees -> stores isn't it conceptually simpler to talk about a "relative path" rather than a "part of a URI body"? | |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp | ||
38 | mention error return (or return Optional) | |
54 | this should check the scheme too - it must be file | |
127 | nit: indexedprojectroot -> indexroot, unless there's some reason for the asymmetry? it seems like the same concept in the opposite direction | |
134–135 | how do you want to handle the case where the path is in fact not relative stored under the root? | |
142 | Don't try to parse if it's not a URI (starts with < or "). | |
166 | you seem to have this check in a few places - I'd suggest it should be in the relativePathToURI function and there should be some plan for handling errors. | |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h | ||
15 | nit: llvm -> llvm-project | |
46 | again, one particular overload is not really the place for this comment, I think. (File comment looks good) | |
55–56 | this also needs the index root, for the proximity path (I'm not sure why they're not URIs, that's unfortunate...) | |
clang-tools-extra/clangd/index/remote/server/Server.cpp | ||
49 | please pass this in as a constructor param rather than accessing the flag directly | |
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp | ||
31 | The File scheme is special. Using a different URI scheme here when we only support file in production is confusing and seems like a last resort to be used when we can't make it work any other way. | |
84 | At this point I really think we should be writing these symbols out instead of trying to parse them from C++ - it's pretty hard to see exactly what fields are set and what is tested when the test is so highly factored. If you like this approach think everything new is covered in other tests, I'd just remove the changes to this one. | |
102 | sorry, I guess I meant testRoot("/"). | |
190 | auto -> std::string (here and in other places where the type is neither overly unreadable nor spelled in the function name) | |
190 | it's not great practice to be assembling the test data from prefix + relative if the code we're testing is splitting it into prefix + relative - easy for the same bugs to cancel each other out. Is testPath("remote/dir/MarshallingTests.cpp") directly too long? | |
194 | (I'd suggest a helper testPathURI("remote/dir/MarshallingTests.cpp") for this) | |
198 | hmm, I don't think that's the reason. | |
214 | nit: use a file path rather than directory? |
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp | ||
---|---|---|
31 | This is dealing with the consequences of test infrastructure being set up in a way that makes all URIs in Symbols and Refs use "unittest" scheme, this helper simply translates those URIs into "file" URIs which are the ones being supported in production. I can not understand why this is not desirable but I'll just clear all URIs and leave those two tests as sanity checks. | |
198 | Ah, llvm::sys::path::append already does that for me. Okay, makes sense to remove this then. |
Address the rest of the comments and refactor code.
Patch is ready for the review again.
Rest is just nits, thanks!
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp | ||
---|---|---|
38 | these functions are nice. It might be clearer to expose them and test them directly, particularly for error cases. Up to you. | |
39 | can we add an assert that the index root is valid? (ends with a platform-appropriate slash) | |
45 | message needs a bit more context. Remote index client got absolute path from server: {0}? | |
50 | nit: lots of const auto throughout - we tend not to use const for local values (as opposed to references or pointers) so this is a bit confusing, particularly when combined with auto. Up to you though. | |
57 | can we add an assert that IndexRoot is valid? | |
60 | Similarly: Remote index got bad URI from client: ... | |
64 | I think it's fine to fold this into the previous check: "bad URI" covers it | |
68 | This isn't a valid IndexRoot, we should assert rather than handle this case I think. | |
131 | shouldn't this return llvm::None, too? a SymbolLocation isn't valid without a path. | |
145 | nit: isLiteralInclude in Headers.h (sorry, had forgotten about this func). | |
149 | isn't the rest of this uriToRelativePath? | |
210 | {} with no index is a nice idea but I think not actually supported | |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h | ||
19 | nit "is passed passed to the remote server". | |
20 | "It contains absolute path to the project root... relative path has unix slashes" can we pull this out into a separate paragraph at the end? The rest is describing the flow, and this describes an API detail. | |
20 | nit: "*the* absolute path", "*the* relative path" etc. |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp | ||
---|---|---|
64 | But it has different message :( ParsedURI.takeError wouldn't make sense here. |
This is a bit low-level - it describes what is being done but not really why. As a caller of this function it doesn't really tell me what I should pass.
Suggest something like "ProjectRoot is an absolute path on the local machine to the source tree described by the remote index. Paths returned by the index will be treated as relative to this directory."
I'd also suggest calling this IndexRoot instead of ProjectRoot, because the latter is a fairly overloaded concept and they need not coincide.