nridge added a comment. Thanks for the update! I have a couple of more minor suggestions, hope you don't mind; I'm trying to make sure the next person to look at this will easily understand what the code is trying to do.
================ Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:112 + // incorrect placeholder `$0` which should have been a normal parameter. + bool ShouldPatchPlaceholder0 = CompletingPattern && [CursorKind] { + // The process of CCR construction employs `clang::getCursorKindForDecl` to ---------------- Can we factor this out to a namespace scope function `bool shouldPatchPlaceholder0(CodeCompletionResult::ResultKind ResultKind, CXCursorKind CursorKind)`? Then the code here becomes pretty simple: ``` unsigned CursorSnippetArg = std::numeric_limits<unsigned>::max(); if (shouldPatchPlaceholder0(ResultKind, CursorKind)) { CursorSnippetArg = count_if(...); } ``` And we can combine some of the comments here into a single comment that explains what we are doing: ``` // By default, the final cursor position is at the end of the snippet, // but we have the option to place it somewhere else using $0. // If the snippet contains a group of statements, we replace the // last placeholder with $0 to leave the cursor there, e.g. // namespace ${1:name} { // ${0:decls} // } // We try to identify such cases using the ResultKind and CursorKind. ``` ================ Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:213 ++SnippetArg; - if (SnippetArg == CursorSnippetArg) { + if (ShouldPatchPlaceholder0 && SnippetArg == CursorSnippetArg) { // We'd like to make $0 a placeholder too, but vscode does not support ---------------- I realized the first part of the condition is now redundant: if `ShouldPatchPlaceholder0 == false`, then `CursorSnippetArg` will have value `std::numeric_limits<unsigned>::max()` and `SnippetArg == CursorSnippetArg` will never be true. ================ Comment at: clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp:30 Snippet.clear(); - getSignature(CCS, &Signature, &Snippet, /*RequiredQualifier=*/nullptr, - CompletingPattern); + // Note that `getSignature` uses CursorKind to identify if we shouldn't + // complete $0 for certain patterns, such as constructors. Passing ---------------- Suggestion: it may be easier to understand if we change the `bool CompletingPattern = false` parameter of this function to `CodeCompletionResult::ResultKind ResultKind = CodeCompletionResult::ResultKind::RK_Declaration`. (And the call site which passed `CompletingPattern=true` can now pass `ResultKind=RK_Pattern`.) Then we don't need to refer to historical behaviour. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145319/new/ https://reviews.llvm.org/D145319 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits