[PATCH] D29451: Add a prototype for clangd v0.1
Jusufadis Bakamovic via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 3 03:37:50 PST 2017
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<char> 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<char> 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<llvm::yaml::ScalarNode>(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 <typename T>
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<T*>(this)->handleMethod(Params, ID);
}
void handleNotification(const llvm::yaml::MappingMode *Params) {
static_cast<T*>(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> {
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: This is extremely repetetive and ugly. Is there a better way?
+//
----------------
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;
};
```
https://reviews.llvm.org/D29451
More information about the cfe-commits
mailing list