ilya-biryukov added a comment. Looks good overall, just a bunch of comments before accepting the revision. Mostly minor code style issues, sorry for spamming you with those.
================ Comment at: clangd/ClangdLSPServer.h:30 ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, + const bool SnippetCompletions, llvm::Optional<StringRef> ResourceDir); ---------------- Code style: we don't use `const` for non-reference parameters. ================ Comment at: clangd/ClangdUnit.cpp:275 +std::string SnippetEscape(const llvm::StringRef Text) { + std::string Result; ---------------- Code style: function names are `lowerCamelCase`. Also maybe rename to `escapeSnippet` to follow the style guide: "function names should be verb phrases, e.g. `openFile()` or `isFoo()`" (see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) ================ Comment at: clangd/ClangdUnit.cpp:315 +protected: + std::vector<CompletionItem> &Items; + std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator; ---------------- Code style: we put all field definitions at the bottom of the class in clangd. Also: why not make the fields private? ================ Comment at: clangd/ClangdUnit.cpp:376 + + CompletionItem Item{InsertTextFormat::PlainText}; + ---------------- Implementations of this function in `PlainTextCompletionItemsCollector` and `SnippetCompletionItemsCollector` are the same. Maybe make `ProcessChunks` virtual instead? Or maybe consider replacing inheritance with a `bool` flag. Inheritance does not seem to buy us much here. This looks simpler: ``` if (EnableSnippets) ProcessChunksWithSnippets(CCS, Item); else ProcessChunksWithoutSnippets(CCS, Item); ``` ================ Comment at: clangd/ClangdUnit.cpp:454 + // a declarator or macro. + Item.filterText = Chunk.Text; + case CodeCompletionString::CK_Text: ---------------- Maybe add a comment that fallthrough to the next case is intended? It's very easy to miss that there's no break intentionally when reading the code. ================ Comment at: clangd/ClangdUnit.cpp:533 + Item.insertText += Chunk.Text; + // Don't even add a space to the label. + } ---------------- Maybe add `break` here? To avoid unintentional fallthrough if new `case` statement gets added. ================ Comment at: clangd/Protocol.h:393 + CompletionItem() = default; + CompletionItem(const InsertTextFormat Format); + ---------------- Maybe remove these constructors? It seems weird that `insertTextFormat` is initialized on construction, while all other fields are filled up later. Having to write all the fields explicitly is pretty low-level, but it's consistent and is done in all other structs from `Protocol.h`. ================ Comment at: clangd/tool/ClangdMain.cpp:31 +static llvm::cl::opt<bool> EnableSnippets( + "enable-snippets", + llvm::cl::desc("Present completions with embedded snippets instead of " ---------------- I've managed to get snippet completion working in VSCode. The problem is that we're using outdated version of `vscode-languageclient` dependency in our npm module. Simply bumping versions of dependencies in `clangd\clients\clangd-vscode\package.json` worked. Had to change ``` "dependencies": { "vscode-languageclient": "^2.6.3", "vscode-languageserver": "^2.6.2" }, ``` to ``` "dependencies": { "vscode-languageclient": "^3.3.0", "vscode-languageserver": "^3.3.0" }, ``` Maybe include this in this commit and make `-enable-snippets` true by default? https://reviews.llvm.org/D37101 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits