sammccall added a comment.
> 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
- 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.
================
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}",
----------------
aren't all these placeholders incorrect? first is {0}
================
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) {
----------------
missing definition is perfectly valid, so both the old and new code look wrong
here
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:310
+ if (!Location) {
+ elog("Can not convet Reference to Potobuf (invalid location) {1}: {2}",
+ From, From.Location);
----------------
convert, protobuf
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:333
}
if (llvm::sys::path::is_relative(IndexRoot)) {
+ elog("Remote index client got a relative path as index root: {1}",
----------------
we shouldn't be checking this every time, IndexRoot is fixed for the lifetime
of the index so it should be checked once and asserted here.
(Wrapping the marshalling in a class is a nice clean way to do this)
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:349
assert(!IndexRoot.empty());
if (llvm::sys::path::is_relative(IndexRoot)) {
+ elog("Index root {1} is not absolute path", IndexRoot);
----------------
and here... IndexRoot shouldn't be checked every time
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83826/new/
https://reviews.llvm.org/D83826
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
