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 

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 
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)

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to