[PATCH] D29451: Add a prototype for clangd v0.1

[Jusufadis Bakamovic via Phabricator via cfe-commits]

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