[PATCH] D83826: [clangd] Don't send invalid messages from remote index

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev marked 4 inline comments as done.
kbobyrev added a comment.

In D83826#2152857 <https://reviews.llvm.org/D83826#2152857>, @sammccall wrote:

> > Also add more error messages and logs.
>
> I'm not sure about the error-handling strategy here:
>
> - it's very verbose in the marshalling code. Are we sure it pays for itself? For comparison, the marshalling code for LSP itself silently returns `false` on error, which is handled at the top level. It's been fine so far.
> - some errors are being logged that are not errors. e.g. it's not an error to drop a symbol because its declaration is in a file outside the index root, that's just expected behavior


Can you give an example of this? Missing d

> - it's not clear which levels are responsible for logging errors, leaving the possibility that some errors will be logged twice or not at all
> - it's not easy to change if we want a slightly different strategy
> 
>   I'd suggest leaving out detailed error reporting for now, and reporting at the top level. You may need some mechanism to distinguish invalid vs filtered-out data. It could be something like a "filtered out" flag that causes the error reporting to be suppressed. There are more design options here if marshalling is a class I think.

Fair enough, will do. Thanks!



================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:151
       !Message.has_canonical_declaration()) {
-    elog("Cannot convert Symbol from Protobuf: {0}",
+    elog("Cannot convert Symbol from Protobuf (missing info, definition or "
+         "declaration): {1}",
----------------
sammccall wrote:
> aren't all these placeholders incorrect? first is {0}
I think they still work for some reason. Anyway, starting with `{0}` is OK, I think they're just off in some places.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:167
   Result.Scope = Message.scope();
   auto Definition = fromProtobuf(Message.definition(), Strings, IndexRoot);
+  if (!Definition) {
----------------
sammccall wrote:
> missing definition is perfectly valid, so both the old and new code look wrong here
Oh, you're right missing definition is indeed OK, but having an invalid one should still be invalid. I guess I should check if it's empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83826