Fix for bug https://bugs.llvm.org/show_bug.cgi?id=34559
Also log unknown fields instead of aborting the JSON parsing because it's
common that new optional fields are added either in new versions of the protocol
or extensions.
Details
Diff Detail
- Build Status
Buildable 10239 Build 10239: arc lint + arc unit
Event Timeline
Totally makes sense to recover by ignoring unknown fields.
Could you also add a test-case?
I couldn't find "code" : null in LSP, is that some kind of extension? What does it do?
clangd/Protocol.cpp | ||
---|---|---|
598 | Let's add a FIXME: to log ignored fields. (We don't pass JSONOutput here, which is used for logging, so doing an actual logging peoperly requires some refactoring). |
clangd/Protocol.cpp | ||
---|---|---|
598 | I'd like to fix all occurrences of this problem since I've had this problem in other places too. I think it makes sense to log ignored fields. Do you think we should pass JSONOutput everywhere? It feels odd. How about passing the llvm::raw_ostream? |
clangd/Protocol.cpp | ||
---|---|---|
598 | I guess since JSONOutput takes care of the thead-safety, we should use it. |
[clangd] Fix codeAction not decoded properly when sent from some clients
Summary: Fix for bug https://bugs.llvm.org/show_bug.cgi?id=34559
Also log unknown fields instead of aborting the JSON parsing because it's
common that new optional fields are added either in new versions of the protocol
or extensions.
Subscribers: ilya-biryukov
Differential Revision: https://reviews.llvm.org/D37754
Could you please clang-format the code?
Overall looks good, just a few comments, mostly regarding code style.
clangd/Protocol.cpp | ||
---|---|---|
27 | Maybe use llvm::StringRef instead of clang::StringRef for constistency with other parts of the file? | |
27 | StringRef is cheap to copy and designed to be passed by value. Just use StringRef instead of const StringRef&. | |
29 | KeyValue.str().c_str() can be replaced with simply KeyValue. | |
598 | Sure, ideally we don't want to have access to JSONOutput::writeMessage, but we do want thread-safety for logging. |
clangd/Protocol.cpp | ||
---|---|---|
27 | I will blame CDT's extract method refactoring for this one :) Done. |
Would you like to wait until https://reviews.llvm.org/D37972 has landed? Or is it OK to update this part with the new logger in a subsequent patch?
No need to, just submit your change. I'll update D37972 accordingly after your change lands.
Maybe use llvm::StringRef instead of clang::StringRef for constistency with other parts of the file?