This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add parantheses while auto-completing functions.
ClosedPublic

Authored by kadircet on Aug 16 2018, 2:24 AM.

Details

Summary

Currently we only add parantheses to the functions if snippets are
enabled, which also inserts snippets for parameters into parantheses. Adding a
new option to put only parantheses. Also it moves the cursor within parantheses
or at the end of them by looking at whether completion item has any parameters
or not. Still requires snippets support on the client side.

Event Timeline

kadircet created this revision.Aug 16 2018, 2:24 AM
ioeric added inline comments.Aug 16 2018, 6:08 AM
clangd/CodeComplete.h
86

IIRC, the goal of this patch is to allow disabling snippet templates for function parameters while still allow appending () or ($0) to function candidates. If that's still the case, I think what we want is probably an option like DisableSnippetTemplateForFunctionArgs?

kadircet updated this revision to Diff 161023.Aug 16 2018, 6:59 AM
kadircet marked an inline comment as done.
  • Change option name.
clangd/CodeComplete.h
86

Yeah, that makes totally more sense. Thanks!

ilya-biryukov added inline comments.Aug 17 2018, 6:45 AM
clangd/CodeComplete.h
86
  • Maybe use positive option names to avoid double negation?
  • Maybe also use a shorter name? E.g. removing 'template' and 'disable' from the name gives a name that does not look any worse: FunctionArgSnippets.
ioeric added inline comments.Aug 17 2018, 6:55 AM
clangd/CodeComplete.cpp
1377

There seems to be no guarantee on whether the snippet is function arg template when SnippetSuffix.size() > 2. A heuristic we could use is probably checking that the template starts with ( and ends with )? Alternatively, we could try to avoid generating the proper snippet according to the flag when we still have the function declaration around. Not sure if this is feasible for index results though.

clangd/CodeComplete.h
86

+1 to a better name. I didn't really give much thought about the name and just wanted to get the idea across :)

kadircet updated this revision to Diff 161243.Aug 17 2018, 7:32 AM
  • Change option name.
kadircet marked 3 inline comments as done.Aug 17 2018, 7:32 AM
kadircet added inline comments.
clangd/CodeComplete.cpp
1377

Actually I was first trying to directly look at function argument count as you mentioned, but unfortunately Symbols coming from index don't have any declarations. Therefore it turns back to looking at Signature in this case, which is same with this one. Apart from that, IIUC, SnippetSuffix contains anything only when completion item is a function otherwise it is empty. So IMO this check should be OK.

ioeric added inline comments.Aug 17 2018, 8:06 AM
clangd/CodeComplete.cpp
1377

Apart from that, IIUC, SnippetSuffix contains anything only when completion item is a function otherwise it is empty. So IMO this check should be OK.

This seems to be true for the current use cases, but I suspect the assumption can change when we add more snippet-based completion results in the future e.g. a snippet for if-statement if (${0}) {${1}}.

kadircet updated this revision to Diff 161256.Aug 17 2018, 8:15 AM
  • Only append to snippets of type function or method.
kadircet marked 3 inline comments as done.Aug 17 2018, 8:15 AM
ioeric added inline comments.Aug 17 2018, 8:25 AM
clangd/CodeComplete.cpp
1379

nit: add parentheses around (... == ...)

1380

nit: no braces around one liner.

1381

I think we are missing one case here: !EnableFunctionArgSnippets && (<not function or method>)?

I think we could do something like:

if (!EnableFunctionArgSnippets && (<function or metrhod>))
  // Truncate parameter template
else
  // append snippet like before
kadircet updated this revision to Diff 161265.Aug 17 2018, 8:35 AM
kadircet marked 3 inline comments as done.
  • Resolve discussions.
kadircet added inline comments.Aug 17 2018, 8:35 AM
clangd/CodeComplete.cpp
1381

Thanks!

ioeric accepted this revision.Aug 17 2018, 8:37 AM

lgtm

This revision is now accepted and ready to land.Aug 17 2018, 8:37 AM
This revision was automatically updated to reflect the committed changes.