[PATCH] D29451: Add a prototype for clangd v0.1
Benjamin Kramer via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 6 07:08:14 PST 2017
bkramer marked 19 inline comments as done.
bkramer added a comment.
In https://reviews.llvm.org/D29451#667606, @arphaman wrote:
> This might be a bad question, but is there any particular reason why you didn't use the YAML Traits API for parsing instead of the raw YAML Stream API? In my experience the traits based API is quite convenient for structured data like various options and parameters in this protocol.
It's an excellent question. I actually tried to make YAMLTraits work for this, but it gets annoying due to the dynamic parts of JSONRPC. You have to first parse the method string and then dispatch based on that string. This is not something that the YAMLTraits interface supports. I think it could be extended to allow mixing code written against YAMLParser with YAMLTraits code and then we can clean up the manual code here. Before that happens I want the YAMLParser implementation working, because there are some complicated union types in the Language Server Protocol that might require additional tweaks to the YAML infrastructure.
================
Comment at: clangd/ClangDMain.cpp:52
+ llvm::StringRef LineRef(Line);
+ if (LineRef.trim().empty())
+ continue;
----------------
djasper wrote:
> Nit: string also has "empty()", right? Maybe only convert to StringRef after this check.
But string doesn't have trim(). An "empty" line still has a \n, which I want to discard.
================
Comment at: clangd/ClangDMain.cpp:65
+ // Now read the JSON.
+ std::vector<char> JSON;
+ JSON.resize(Len);
----------------
klimek wrote:
> Adi wrote:
> > Avoid unnecessary JSON.resize(Len) & potential reallocation during JSON.push_back('\0') by allocating enough space in advance: std::vector<char> JSON(Len + 1);
> Shouldn't that be unsigned char for raw bytes?
I agree, but that would add pointer punning on every usage, as iostreams and YAML parser want normal chars.
================
Comment at: clangd/ClangDMain.cpp:73-77
+ // Log the message.
+ Logs << "<-- ";
+ Logs.write(JSON.data(), JSON.size());
+ Logs << '\n';
+ Logs.flush();
----------------
Adi wrote:
> Since llvm::StringRef does not own the data, I think it would make more sense to do the logging after Dispatcher.call().
But that will confuse the logging. You'd get the query after the answer.
================
Comment at: clangd/ClangDMain.cpp:74
+ // Log the message.
+ Logs << "<-- ";
+ Logs.write(JSON.data(), JSON.size());
----------------
djasper wrote:
> So you currently always print this to std::err.. Should you guard this in DEBUG or something?
My goal is to wire this up to the editor in some way and make it possible to discard the log with a command line flag (send it to nulls()). I think using DEBUG only makes that more complicated.
That said, the logging parts of clangd probably need a redesign at some point ;)
================
Comment at: clangd/ClangDMain.cpp:80
+ // Finally, execute the action for this JSON message.
+ Dispatcher.call(llvm::StringRef(JSON.data(), JSON.size() - 1));
+ }
----------------
Adi wrote:
> Adi wrote:
> > You are building llvm::StringRef without the trailing null byte here, yet comment above states that YAML parser requires a null byte.
> Perhaps make a log entry if Dispatcher.call() failed.
This is intentional. YAML parser treats the pointer inside of the StringRef as a null terminated string, reading past the end of the StringRef.
================
Comment at: clangd/DocumentStore.h:25
+ /// Add a document to the store. Overwrites existing contents.
+ void setDocument(StringRef Uri, StringRef Text) { Docs[Uri] = Text; }
+ /// Delete a document from the store.
----------------
djasper wrote:
> I'd call this addDocument for symmetry with removeDocument. The fact that you phrase the comment that way, also make me think that this will be the more intuitive name.
I picked set because it can overwrite contents and that's how it used. Your comment makes sense though so I changed it back to add.
================
Comment at: clangd/JSONRPCDispatcher.cpp:32-40
+ Logs << "Method ignored.\n";
+ // Return that this method is unsupported.
+ writeMessage(
+ R"({"jsonrpc":"2.0","id":)" + ID +
+ R"(,"error":{"code":-32601}})");
+}
+
----------------
Adi wrote:
> I would extract this implementation to the separate handler class (e.g. OperationNotSupportedHandler) and make this an interface class. It would make code and intent more explicit.
The goal of the default methods is to make it easy to implement only one of the two methods. If I want to do that with an OperationNotSupportedHandler I'd have to inherit from that, which is more awkward.
================
Comment at: clangd/JSONRPCDispatcher.cpp:82-83
+ // This should be "2.0". Always.
+ Version = dyn_cast<llvm::yaml::ScalarNode>(Value);
+ if (Version->getRawValue() != "2.0")
+ return false;
----------------
Adi wrote:
> dyn_cast might fail and leave you with Version set to nullptr. Using static_cast is better approach if you are sure the cast will be successful.
It's supposed to not crash and return an error. This is user input. Added the missing nullptr check.
================
Comment at: clangd/JSONRPCDispatcher.cpp:89
+ Id = dyn_cast<llvm::yaml::ScalarNode>(Value);
+ } else if (KeyValue == "params") {
+ if (!Method)
----------------
djasper wrote:
> I'd pull out this case and move it to the front to show that this is a guaranteed early exit and the function can never fall through to the following code. But I am not sure.
Not sure if I understand. It's vital that this part is called when we just read params, otherwise the YAMLParser will crash.
================
Comment at: clangd/JSONRPCDispatcher.h:29
+ /// a result on Outs.
+ virtual void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID);
+ /// Called when the server receives a notification. No result should be
----------------
klimek wrote:
> klimek wrote:
> > Adi wrote:
> > > const ptr/ref to Params?
> > Can we make those Params pointers-to-const?
> Here and below, document what the default implementations do.
YAML nodes are supposed to be immutable and don't provide const members.
================
Comment at: clangd/JSONRPCDispatcher.h:29-32
+ virtual void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID);
+ /// Called when the server receives a notification. No result should be
+ /// written to Outs.
+ virtual void handleNotification(llvm::yaml::MappingNode *Params);
----------------
bkramer wrote:
> klimek wrote:
> > klimek wrote:
> > > Adi wrote:
> > > > const ptr/ref to Params?
> > > Can we make those Params pointers-to-const?
> > Here and below, document what the default implementations do.
> YAML nodes are supposed to be immutable and don't provide const members.
No. YAML nodes are supposed to be immutable and don't provide const members.
================
Comment at: clangd/Protocol.cpp:11
+// This file contains the serialization code for the LSP structs.
+// FIXME: This is extremely repetetive and ugly. Is there a better way?
+//
----------------
Adi wrote:
> Adi wrote:
> > To some degree it could be refactored. I would go for CRTP again here. It would simplify the code a little bit. E.g.
> >
> > ```
> > template <typename T>
> > struct Params {
> > static llvm::Optional<T> parse(const llvm::yaml::MappingNode& node) {
> > T Result;
> > for (auto& NextKeyValue : node) {
> > auto *KeyString = dyn_cast<llvm::yaml::ScalarNode>(NextKeyValue.getKey());
> > if (!KeyString)
> > return llvm::None;
> >
> > llvm::SmallString<10> KeyStorage;
> > StringRef KeyValue = KeyString->getValue(KeyStorage);
> > auto *Value = dyn_cast_or_null<llvm::yaml::ScalarNode>(NextKeyValue.getValue());
> > if (!Value)
> > return llvm::None;
> >
> > Result = static_cast<T*>(this)->setParam(KeyValue, *Value); // or make Result an argument if worried about RVO
> > if (!Result) // we can make this more neat by placing it inside a for loop condition (ranged-for doesn't allow this)
> > return llvm::none;
> > }
> > return Result;
> > }
> > };
> >
> > struct TextDocumentIdentifier : public Params<TextDocumentIdentifier> {
> > /// The text document's URI.
> > std::string uri;
> >
> > llvm::Optional<TextDocumentIdentifier> setParam(StringRef KeyValue, const llvm::yaml::ScalarNode& Value) {
> > TextDocumentIdentifier Result;
> > llvm::SmallString<10> Storage;
> > if (KeyValue == "uri") {
> > Result.uri = Value.getValue(Storage);
> > } else if (KeyValue == "version") {
> > // FIXME: parse version, but only for VersionedTextDocumentIdentifiers.
> > } else {
> > return llvm::None;
> > }
> > return Result;
> > }
> > };
> >
> > struct Position : public Params<Position> {
> > /// Line position in a document (zero-based).
> > int line;
> >
> > /// Character offset on a line in a document (zero-based).
> > int character;
> >
> > llvm::Optional<Position> setParam(StringRef KeyValue, const llvm::yaml::ScalarNode& Value) {
> > Position Result;
> > llvm::SmallString<10> Storage;
> > if (KeyValue == "line") {
> > long long Val;
> > if (llvm::getAsSignedInteger(Value.getValue(Storage), 0, Val))
> > return llvm::None;
> > Result.line = Val;
> > } else if (KeyValue == "character") {
> > long long Val;
> > if (llvm::getAsSignedInteger(Value.getValue(Storage), 0, Val))
> > return llvm::None;
> > Result.character = Val;
> > } else {
> > return llvm::None;
> > }
> > return Result;
> > }
> > };
> > ```
> >
> > I am not really familiar with all of the dependencies here but I may suggest that one might also try to generalize this approach even further by encapsulating the handling of `KeyValue` into the common implementation. E.g.
> >
> > ```
> > template <typename T>
> > T fromKeyValueToParam(StringRef KeyValue, const llvm::yaml::ScalarNode& Value) {
> > T Result;
> > llvm::SmallString<10> Storage;
> >
> > if (KeyValue == "line")
> > long long Val;
> > if (llvm::getAsSignedInteger(Value.getValue(Storage), 0, Val))
> > return llvm::None;
> > Result.line = Val;
> > } else if (KeyValue == "character") {
> > long long Val;
> > if (llvm::getAsSignedInteger(Value.getValue(Storage), 0, Val))
> > return llvm::None;
> > Result.character = Val;
> > }
> > else if (KeyValue == "textDocument") {
> > auto Parsed = TextDocumentIdentifier::parse(Value);
> > if (!Parsed)
> > return llvm::None;
> > Result.textDocument = std::move(*Parsed);
> > }
> > else if (KeyValue == "options") {
> > auto Parsed = FormattingOptions::parse(Value);
> > if (!Parsed)
> > return llvm::None;
> > Result.options = std::move(*Parsed);else if (KeyValue == "range")
> > }
> > else if (KeyValue == "range") {
> > auto Parsed = Range::parse(Value);
> > if (!Parsed)
> > return llvm::None;
> > Result.range = std::move(*Parsed);
> > }
> > else if ( ... ) {
> > ...
> > }
> >
> > ...
> >
> > return Result;
> > }
> > ```
> >
> > Then you would be able to handle everything in `Params<T>::parse()` without any other specialized code required to be written in concrete parameters. Code could look something like this:
> >
> > ```
> > template <typename T>
> > struct Params {
> > static llvm::Optional<T> parse(const llvm::yaml::MappingNode& Params) {
> > T Result;
> > for (auto& NextKeyValue : Params) {
> > auto *KeyString = dyn_cast<llvm::yaml::ScalarNode>(NextKeyValue.getKey());
> > if (!KeyString)
> > return llvm::None;
> >
> > llvm::SmallString<10> KeyStorage;
> > StringRef KeyValue = KeyString->getValue(KeyStorage);
> > auto *Value = dyn_cast_or_null<llvm::yaml::ScalarNode>(NextKeyValue.getValue());
> > if (!Value)
> > return llvm::None;
> >
> > Result = fromKeyValueToParam(KeyValue, *Value);
> > if (!Result) // we can make this more neat by placing it inside a for loop condition (ranged-for doesn't allow us that)
> > return llvm::none;
> > }
> > return Result;
> > }
> > };
> >
> > struct Position : public Params<Position> {
> > /// Line position in a document (zero-based).
> > int line;
> >
> > /// Character offset on a line in a document (zero-based).
> > int character;
> > };
> >
> > struct TextDocumentIdentifier : public Params<TextDocumentIdentifier> {
> > /// The text document's URI.
> > std::string uri;
> > };
> > ```
> `fromKeyValueToParam()` will of course not compile but you've got the idea. I think it can be achieved in this way somehow.
I don't think this really helps. The most annoying repetitive part is the walk over the fields, which is still there.
In the long term I want to replace this with a YAMLTraits-like thing, but that will take a while.
================
Comment at: test/clangd/formatting.txt:9
+# CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
+# CHECK: "textDocumentSync": 1,
+# CHECK: "documentFormattingProvider": true,
----------------
djasper wrote:
> How did you come up with this indentation?
It's hardcoded as a raw string literal in the source. It looks better there ;)
https://reviews.llvm.org/D29451
More information about the cfe-commits
mailing list