[PATCH] D29451: Add a prototype for clangd v0.1
Adi added a comment.
I had some spare time ... I hope you don't mind the comments. Hopefully you
will find something useful :)
Comment at: clangd/ClangDMain.cpp:65-70
+std::vector JSON;
+JSON.resize(Len);
+std::cin.read(JSON.data(), Len);
+
+// Insert a trailing null byte. YAML parser requires this.
+JSON.push_back('\0');
Avoid unnecessary JSON.resize(Len) & potential reallocation during
JSON.push_back('\0') by allocating enough space in advance: std::vector
JSON(Len + 1);
Comment at: clangd/ClangDMain.cpp:73-77
+ // Log the message.
+ Logs << "<-- ";
+ Logs.write(JSON.data(), JSON.size());
+ Logs << '\n';
+ Logs.flush();
Since llvm::StringRef does not own the data, I think it would make more sense
to do the logging after Dispatcher.call().
Comment at: clangd/ClangDMain.cpp:80
+ // Finally, execute the action for this JSON message.
+ Dispatcher.call(llvm::StringRef(JSON.data(), JSON.size() - 1));
+}
You are building llvm::StringRef without the trailing null byte here, yet
comment above states that YAML parser requires a null byte.
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:
> 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.
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}})");
+}
+
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.
Comment at: clangd/JSONRPCDispatcher.cpp:82-83
+ // This should be "2.0". Always.
+ Version = dyn_cast(Value);
+ if (Version->getRawValue() != "2.0")
+return false;
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.
Comment at: clangd/JSONRPCDispatcher.cpp:112-120
+ if (!Method)
+return false;
+ llvm::SmallString<10> MethodStorage;
+ auto I = Handlers.find(Method->getValue(MethodStorage));
+ auto &Handler = I != Handlers.end() ? I->second : UnknownHandler;
+ if (Id)
+Handler->handleMethod(nullptr, Id->getRawValue());
Perhaps refactor this code and the one above into the separate method to gain
some more readability.
Comment at: clangd/JSONRPCDispatcher.h:25-32
+ virtual ~Handler() = default;
+
+ /// Called when the server receives a method call. This is supposed to return
+ /// a result on Outs.
+ virtual void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID);
+ /// Called when the server receives a notification. No result should be
+ /// written to Outs.
To avoid virtual dispatch one may rewrite this class in terms of CRTP. E.g.
```
template
class Handler {
public:
Handler(llvm::raw_ostream &Outs, llvm::raw_ostream &Logs)
: Outs(Outs), Logs(Logs) {}
virtual ~Handler() = default;
void handleMethod(const llvm::yaml::MappingMode *Params, StringRef ID) {
static_cast(this)->handleMethod(Params, ID);
}
void handleNotification(const llvm::yaml::MappingMode *Params) {
static_cast(this)->handleNotification(Params);
}
protected:
llvm::raw_ostream &Outs;
llvm::raw_ostream &Logs;
void writeMessage(const Twine &Message);
};
```
And then use it as:
```
struct MyConcreteHandler : public Handler {
MyConcreteHandler(llvm::raw_ostream &Outs, llvm::raw_ostream &Logs /* other
params if necessary */)
: Handler(Outs, Logs) /* init other members */
{}
void handleMethod(const llvm::yaml::MappingMode *Params, StringRef ID) {
// impl
}
void handleNotification(const llvm::yaml::MappingMode *Params) {
// impl
}
};
```
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
const ptr/ref to Params?
Comment at: clangd/JSONRPCDispatcher.h:32
+ /// written to Outs.
+ virtual void handleNotification(llvm::yaml::MappingNode *Params);
+
const ptr/ref to Params?
Comment at: clangd/Protocol.cpp:11
+// This file contains the serialization code for the LSP structs.
+// FIXME
[PATCH] D29451: Add a prototype for clangd v0.1
Adi added inline comments.
Comment at: clangd/JSONRPCDispatcher.h:25-32
+ virtual ~Handler() = default;
+
+ /// Called when the server receives a method call. This is supposed to return
+ /// a result on Outs.
+ virtual void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID);
+ /// Called when the server receives a notification. No result should be
+ /// written to Outs.
klimek wrote:
> Adi wrote:
> > To avoid virtual dispatch one may rewrite this class in terms of CRTP. E.g.
> >
> > ```
> > template
> > class Handler {
> > public:
> > Handler(llvm::raw_ostream &Outs, llvm::raw_ostream &Logs)
> > : Outs(Outs), Logs(Logs) {}
> > virtual ~Handler() = default;
> >
> > void handleMethod(const llvm::yaml::MappingMode *Params, StringRef ID)
> > {
> > static_cast(this)->handleMethod(Params, ID);
> > }
> > void handleNotification(const llvm::yaml::MappingMode *Params) {
> > static_cast(this)->handleNotification(Params);
> > }
> >
> > protected:
> > llvm::raw_ostream &Outs;
> > llvm::raw_ostream &Logs;
> >
> > void writeMessage(const Twine &Message);
> > };
> > ```
> > And then use it as:
> >
> > ```
> > struct MyConcreteHandler : public Handler {
> > MyConcreteHandler(llvm::raw_ostream &Outs, llvm::raw_ostream &Logs /*
> > other params if necessary */)
> > : Handler(Outs, Logs) /* init other members */
> > {}
> >
> > void handleMethod(const llvm::yaml::MappingMode *Params, StringRef ID)
> > {
> > // impl
> > }
> > void handleNotification(const llvm::yaml::MappingMode *Params) {
> > // impl
> > }
> > };
> > ```
> > To avoid virtual dispatch one may rewrite this class in terms of CRTP. E.g.
>
> Why would virtual dispatch be a problem here? It seems strictly simpler this
> way.
>
I've mentioned it only as an alternative approach. I was not implying that
virtual dispatch is a problem. It's a matter of taste in this case really.
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:
> 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
> struct Params {
> static llvm::Optional parse(const llvm::yaml::MappingNode& node) {
> T Result;
> for (auto& NextKeyValue : node) {
> auto *KeyString =
> dyn_cast(NextKeyValue.getKey());
> if (!KeyString)
> return llvm::None;
>
> llvm::SmallString<10> KeyStorage;
> StringRef KeyValue = KeyString->getValue(KeyStorage);
> auto *Value =
> dyn_cast_or_null(NextKeyValue.getValue());
> if (!Value)
> return llvm::None;
>
> Result = static_cast(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 {
> /// The text document's URI.
> std::string uri;
>
> llvm::Optional 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 {
> /// Line position in a document (zero-based).
> int line;
>
> /// Character offset on a line in a document (zero-based).
> int character;
>
> llvm::Optional 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
