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