sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Rest is just nits, thanks!
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:38
+/// provided by the client.
+llvm::Optional<std::string> relativePathToURI(llvm::StringRef RelativePath,
+ llvm::StringRef IndexRoot) {
----------------
these functions are nice. It might be clearer to expose them and test them
directly, particularly for error cases. Up to you.
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:39
+llvm::Optional<std::string> relativePathToURI(llvm::StringRef RelativePath,
+ llvm::StringRef IndexRoot) {
+ assert(RelativePath == llvm::sys::path::convert_to_slash(
----------------
can we add an assert that the index root is valid? (ends with a
platform-appropriate slash)
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:45
+ if (llvm::sys::path::is_absolute(RelativePath)) {
+ elog("{0} is not relative path", RelativePath);
+ return llvm::None;
----------------
message needs a bit more context. `Remote index client got absolute path from
server: {0}`?
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:50
+ llvm::sys::path::append(FullPath, RelativePath);
+ const auto Result = URI::createFile(FullPath);
+ return Result.toString();
----------------
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.
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:57
+llvm::Optional<std::string> uriToRelativePath(llvm::StringRef URI,
+ llvm::StringRef IndexRoot) {
+ auto ParsedURI = URI::parse(URI);
----------------
can we add an assert that IndexRoot is valid?
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:60
+ if (!ParsedURI) {
+ elog("Can not parse URI {0}: {1}", URI, ParsedURI.takeError());
+ return llvm::None;
----------------
Similarly: `Remote index got bad URI from client: ...`
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:64
+ if (ParsedURI->scheme() != "file") {
+ elog("Can not parse URI with scheme other than \"file\" {0}", URI);
+ return llvm::None;
----------------
I think it's fine to fold this into the previous check: "bad URI" covers it
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:68
+ llvm::SmallString<256> Result = ParsedURI->body();
+ if (IndexRoot.empty())
+ return std::string(Result);
----------------
This isn't a valid IndexRoot, we should assert rather than handle this case I
think.
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:131
+ if (!RelativePath)
+ return Result;
+ *Result.mutable_file_path() = *RelativePath;
----------------
shouldn't this return llvm::None, too? a SymbolLocation isn't valid without a
path.
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:145
+ // If header starts with < or " it is not a URI: do not attempt to parse it.
+ if (Header.front() == '<' || Header.front() == '"') {
+ Result.set_header(Header);
----------------
nit: isLiteralInclude in Headers.h (sorry, had forgotten about this func).
Calling that function probably avoids the need for the comment too.
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:149
+ }
+ auto URI = URI::parse(Header);
+ if (!URI) {
----------------
isn't the rest of this uriToRelativePath?
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:210
if (!ID) {
- elog("Cannot convert parse SymbolID {} from Protobuf: {}", ID.takeError(),
+ elog("Cannot parse SymbolID {} given Protobuf: {}", ID.takeError(),
Message.ShortDebugString());
----------------
{} with no index is a nice idea but I think not actually supported
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h:19
+// path and passes paths relative to the project root over the wire
+// ("include/HelloWorld.h" in this example). The indexed project root is passed
+// passed to the remote server. It contains absolute path to the project root
----------------
nit "is passed passed to the remote server".
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h:20
+// ("include/HelloWorld.h" in this example). The indexed project root is passed
+// passed to the remote server. It contains absolute path to the project root
+// and includes a trailing slash. Relative path passed over the wire has unix
----------------
"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.
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h:20
+// ("include/HelloWorld.h" in this example). The indexed project root is passed
+// passed to the remote server. It contains absolute path to the project root
+// and includes a trailing slash. Relative path passed over the wire has unix
----------------
sammccall wrote:
> "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.
nit: "*the* absolute path", "*the* relative path" etc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82938/new/
https://reviews.llvm.org/D82938
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
