[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clangd/tool/ClangdMain.cpp:197
 
+static llvm::cl::opt<bool> IncludeFixIts(
+    "include-fixits",
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > I wonder if we should make the `IncludeFixIts` option hidden?
> > > It currently only works for our YCM integration, VSCode and other clients break.
> > why would a user want to turn this on or off?
> Ideally, we want to have it always on.
> However, all editors interpret the results we return in different ways. This is a temporary option until we can define how text edits are handled by LSP.
> We filed the bugs, will dig them up on Monday.
+1 to @ilya-biryukov . We might need to wait for a while until all editors supports additionalTextEdits in a sane way. 


================
Comment at: clangd/tool/ClangdMain.cpp:202
+        "Like changing an arrow(->) member access to dot(.) member access."),
+    llvm::cl::init(clangd::CodeCompleteOptions().IncludeFixIts));
+
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > Maybe specify the value here explicitly?
> > > The defaults for users of the clangd binary (what the option is for) is not necessarily the best default for the `ClangdServer` clients (what the default in the struct is for).
> > > More importantly, it's useful to be more straightforward about the defaults we have for the options.
> > Not sure I agree here, this is consistent with the other options above. It's the other `ClangdServer` clients that are the weirdos, they should have to be explicit :-)
> > 
> > The defaults are clear when running with `-help`, which is how users will see them.
> > The defaults are clear when running with -help, which is how users will see them.
> Sure, but I would still argue reading the code is simpler without extra indirection and defaults of the binary are not necessarily the defaults we use in the APIs.
> 
> But also feel free to keep the code as is, don't think it's terribly important.
Keeping it as it is.


================
Comment at: clangd/tool/ClangdMain.cpp:205
+static llvm::cl::opt<bool> EnableFunctionArgSnippets(
+    "enable-function-arg-snippets",
+    llvm::cl::desc("Gives snippets for function arguments, when disabled only "
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > I wonder if we can infer this setting from the `-completion-style' (`=bundled` implies `enable-function-arg-snippets == false`)
> > > @sammccall, WDYT?
> > They're not inextricably linked, the main connection is "these options both need good signaturehelp support to be really useful".
> > But we might want to link them just to cut down on number of options.
> > 
> > I don't have a strong opinion, I don't use `-completion-style=bundled`. Can we find a few people who do and ask if they like this setting?
> I would (obviously) want these two options to be packaged together whenever I use `=bundled`.
> But I also use VSCode, which has signatureHelp.
> 
> I think @ioeric wanted to try out the `=bundled` completion style. @ioeric, WDYT?
ping on this discussion to @ioeric , but I think linking seems like a good idea. Because it wouldn't be very useful if one already selects a specific overload from completion list.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51214