Page MenuHomePhabricator

[clangd] Propagate remote index errors via Expected
ClosedPublic

Authored by kbobyrev on Jul 30 2020, 5:40 AM.

Details

Summary

This is a refactoring: errors should be logged only on the highest level.
Switch from Optional to Expected in the serialization code.

Diff Detail

Event Timeline

kbobyrev created this revision.Jul 30 2020, 5:40 AM
kbobyrev requested review of this revision.Jul 30 2020, 5:40 AM
kbobyrev updated this revision to Diff 281897.Jul 30 2020, 5:41 AM

Client: print debug string for the stream_result, not the ReplyT.

Harbormaster completed remote builds in B66379: Diff 281895.

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

clang-tools-extra/clangd/index/remote/Client.cpp
60

elog already consumes the error

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
132

error mentions definition too but conditional only looks for info and declaration?

159–160

nit: swap branches and early exit, i.e.

if(!SerializedHeader) {
 elog...
 continue;
}
push_back;
160–162

what makes this a non-terminal error?

182–183

nit: redundant braces

267–268

again, what makes this non-terminal?

270

elog already consumes the error

303–306

is RelativePath user provided? can't we just assert on non-emptiness and non-absoluteness and make this function return a std::string instead?

318

again why not make this (and the following) an assertion?

clang-tools-extra/clangd/index/remote/server/Server.cpp
89

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.

94

this still has auto in it? (and other callbacks too)

99

again, elog consumes

123

same for usage of descriptor()->name()

132

again, elog consumes

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

Haha, yeah I thought about that but then looked at other examples and saw that in some cases there are even more verbose calls :)

kbobyrev updated this revision to Diff 281946.Jul 30 2020, 8:42 AM
kbobyrev marked 12 inline comments as done.

Resolve most review comments.

kbobyrev added inline comments.Jul 30 2020, 8:44 AM
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
303–306

Could you elaborate on what you mean by user provided? Relative path is something that comes from the remote index. Ideally, this is non-empty and relative but I think the safer option is to maybe check for errors? Actually, I'm not sure about this but it's not user provided as in "given as a flag passed to CLI invocation by the end user".

I think the point here is making errors recoverable and not shutting down the client, otherwise everything could be an assertion? Same in other places.

WDYT?

kadircet accepted this revision.Jul 31 2020, 2:35 AM

thanks, lgtm!

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
303–306

Could you elaborate on what you mean by user provided? Relative path is something that comes from the remote index.

Yes I was asking whether this comes from outside and ideally not processed at all before arriving at this logic.

Ideally, this is non-empty and relative but I think the safer option is to maybe check for errors? Actually, I'm not sure about this but it's not user provided as in "given as a flag passed to CLI invocation by the end user".

Right, the safer is always to check for errors, but if we've already processed the RelativePath before it reaches this code path then there wouldn't be much value in spending more runtime checks (and also it would be callers responsibility generate the errors in such cases). I was asking this because I wasn't aware of the whole structure, but looks like this is literally the entry point (i.e. the first logic that verifies a user input) for path to uri conversions. So no action needed here.

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
169

i would ASSERT_TRUE here, as you'll end up crashing when fromProtoBuf returns an error (because you are not consuming the error and it is destroyed after completion of the statement).

418

again should be ASSERT_TRUE for similar concerns stated above.

436

again should be ASSERT_TRUE for similar concerns stated above.

This revision is now accepted and ready to land.Jul 31 2020, 2:35 AM
kbobyrev updated this revision to Diff 282166.Jul 31 2020, 2:47 AM
kbobyrev marked 6 inline comments as done.

Good points, thanks!

Addressed post-LGTM comments.

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
303–306

Yes, I agree that spending runtime on this is unfortunate but I guess it's the cost we'd have to pay :(

This revision was landed with ongoing or failed builds.Jul 31 2020, 2:48 AM
This revision was automatically updated to reflect the committed changes.