[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/CodeComplete.cpp:1377
+      // Check whether function has any parameters or not.
+      LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";
+    else
----------------
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.


================
Comment at: clangd/CodeComplete.h:85
+
+  /// Enables cursor to be moved around according to completions needs even when
+  /// snippets are disabled. For example selecting a function with parameters as
----------------
ilya-biryukov wrote:
> kadircet wrote:
> > ioeric wrote:
> > > 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`? 
> > Yeah, that makes totally more sense. Thanks!
> - 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`.
+1 to a better name. I didn't really give much thought about the name and just wanted to get the idea across :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835