[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

i think you might want to have an helper for creating stringerrors and use it in all places



================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:60
+             Response.takeError());
+        llvm::consumeError(Response.takeError());
         ++FailedToParse;
----------------
elog already consumes the error


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:131
+  if (!Message.has_info() || !Message.has_canonical_declaration())
+    return make_error<StringError>("Missing info, definition or declaration.",
+                                   inconvertibleErrorCode());
----------------
error mentions definition too but conditional only looks for info and declaration?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:162
       Result.IncludeHeaders.push_back(*SerializedHeader);
     else
+      elog("Cannot convert HeaderWithIncludes from protobuf; skipping it: {0}. "
----------------
nit: swap branches and early exit, i.e.

```
if(!SerializedHeader) {
 elog...
 continue;
}
push_back;
```


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:163
     else
-      elog("Cannot convert HeaderWithIncludes from protobuf: {0}",
-           Header.DebugString());
+      elog("Cannot convert HeaderWithIncludes from protobuf; skipping it: {0}. "
+           "Reason: {1}",
----------------
what makes this a non-terminal error?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:189
+    return SubjectID.takeError();
   if (!Message.has_object()) {
+    return make_error<StringError>("Missing Object.", inconvertibleErrorCode());
----------------
nit: redundant braces 


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:273
     auto Serialized = toProtobuf(Header);
     if (!Serialized) {
+      elog("Can not convert IncludeHeaderWithReferences {0} to protobuf; "
----------------
again, what makes this non-terminal?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:277
+           Header.IncludeHeader, Serialized.takeError());
+      llvm::consumeError(Serialized.takeError());
       continue;
----------------
elog already consumes the error


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:313
                              RelativePath, llvm::sys::path::Style::posix));
-  if (RelativePath.empty()) {
-    return llvm::None;
-  }
-  if (llvm::sys::path::is_absolute(RelativePath)) {
-    return llvm::None;
-  }
+  if (RelativePath.empty())
+    return make_error<StringError>("Empty relative path.",
----------------
is `RelativePath` user provided? can't we just assert on non-emptiness and non-absoluteness and make this function return a std::string instead?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:330
+    return ParsedURI.takeError();
+  if (ParsedURI->scheme() != "file")
+    return make_error<StringError>("Can not use URI schemes other than file.",
----------------
again why not make this (and the following) an assertion?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:89
     if (!Req) {
-      elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError());
+      elog("Can not parse {0} from protobuf: {1}",
+           LookupRequest::descriptor()->name(), Req.takeError());
----------------
nit: I would keep `LookupRequest` instead of going through `descriptor()->name()` (maybe even change the one in tracer), makes it more explicit especially for the people not experienced with protos.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:95
     unsigned FailedToSend = 0;
     Index->lookup(*Req, [&](const auto &Item) {
       auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
----------------
this still has `auto` in it? (and other callbacks too)


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:100
+             SerializedItem.takeError());
+        llvm::consumeError(SerializedItem.takeError());
         ++FailedToSend;
----------------
again, elog consumes


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:124
+      elog("Can not parse {0} from protobuf: {1}",
+           LookupRequest::descriptor()->name(), Req.takeError());
       return grpc::Status::CANCELLED;
----------------
same for usage of `descriptor()->name()`


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:134
+             SerializedItem.takeError());
+        llvm::consumeError(SerializedItem.takeError());
         ++FailedToSend;
----------------
again, elog consumes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939