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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
