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 <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
> > }
> > };
> > ```
> > 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 <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.
https://reviews.llvm.org/D29451
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
