sammccall added a comment. In https://reviews.llvm.org/D39057#906297, @ilya-biryukov wrote:
> There's another patch (https://reviews.llvm.org/D39276) that tries to add > `workspace/executeCommand` for a slightly different use-case. > Could we take the code for parsing/handling `workspace/executeCommand` from > this patch and extract it into a separate change so that these two patches > can be reviewed independently? @arphaman unless you're far down this path already, I'd actually prefer we land that patch soon and merge this one into it: - It's got a pretty narrow scope (1 command, not much logic), and will be ready soon. It's almost actually the isolated patch that @ilya-biryukov describes. This patch is both more general in scope and is stacked on top of other nontrivial changes, so will probably take longer. - It makes a couple of different decisions that I think would aid the design here, will leave detailed comments. But if people prefer, we can certainly tackle command parsing separately. ================ Comment at: clangd/Protocol.cpp:990 +llvm::Optional<CommandArgument> +CommandArgument::parse(llvm::yaml::MappingNode *Params, + clangd::Logger &Logger) { ---------------- Here we're trying to parse the args independent of what the command is. I'm not sure this is a path we want to go down: - there's no guarantee that args are even Objects, for a start! - we're detecting the type based on field names "range" and "doc", which don't seem that unlikely to be used in different ways by different commands. In that case, we've painted ourselves into a corner. - I'm uncomfortable with the idea that if we encounter an unknown command with unknown args, we're going to throw them at "parse" anyway and... probably get None back? (I'd hope our parsers don't crash, but d0k's fuzzing shows otherwise I think) If we parse the arg list with some small per-command logic, we can still easily share the code that handles common arg types like a selection. ================ Comment at: clangd/Protocol.h:533 +/// Represents an editor command argument. +struct CommandArgument { ---------------- vector<union> is a good literal translation of any[], but it's not that convenient for the code that's consuming these args, nor is it convenient to implement (basically because LLVM lacks a good variant, I think). D39276 hints at a different approach that I think is worth a look: basically we try to fit the args into a set of typed "slots" which are just Optional<T> fields on `EditorCommand`. For each command, we can decide how to parse the args into these slots, and which slots we're going to look at. We can share slots for common attributes (selection), but some of them will probably be specialized. (If size is a concern, we can use unique_ptr instead of Optional). This does mean making some assumptions (relative order of different args isn't significant, have to choose between "single" and "list" for each slot) but they seem pretty safe/flexible. Repository: rL LLVM https://reviews.llvm.org/D39057 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
