sammccall added a comment.
I like the behavior in this patch - I think multiple options is going to be
distracting and crowd out other good results, and the heuristic seems pretty
good.
This needs a test in e.g. `llvm-project/clang/test/CodeCompletion/patterns.cpp`.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5764
+
+ auto AddCodePatterns = [&] {
+ if (IsBracedThen) {
----------------
name could be clearer here: AddElseBodyPattern?
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5773
+ } else {
+ // HorizontalSpace so the hint looks like 'else statement' instead of
+ // 'elsestatement'.
----------------
Hmm, this is in clangd I guess, which ignores vertical space when rendering
snippets.
Maybe that's a bug there, and clangd should include vertical space as space,
but collapse consecutive ones?
I wonder what other consumers do.
If we do keep it (also fine) you could switch the order of the vertical and
horizontal space, which suggests indentation. Up to you though.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5775
+ // 'elsestatement'.
+ // FIXME, would be nicer if there was a CK_IndentationSpace option that
+ // would add the correct number of tabs/spaces.
----------------
I'm not sure about this FIXME, I think it's pretty unlikely this will happen
(this layer of clang is unlikely to retain enough info and know enough about
style to do this, and if it's solved at another layer it's likely to be
clang-format that doesn't require this hint)
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5779
+ Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
+ Builder.AddPlaceholderChunk("statement");
+ }
----------------
do you want to insert the semicolon after "statement"? (i.e. the semicolon is
not part of the placeholder)
This seems likely to play better with e.g. clang-format, and should give more
accurate diagnostics if the user hasn't filled in the placeholders yet.
(I don't see much precedent one way or the other, there's only "statements" in
blocks... the for-loop snippet obviously inserts semicolons though)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82626/new/
https://reviews.llvm.org/D82626
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
