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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 24 08:25:27 PDT 2018


sammccall added a comment.

The description says "LSP clients", i.e. editors.
For editors (e.g. whether editor can handle snippets), LSP capability negotiation rather than flags is the right thing. I suspect this is true for including completion fixes.
For users (e.g. I like behavior X), flags are the right mechanism. But we don't need to expose every option, just the important ones.



================
Comment at: clangd/tool/ClangdMain.cpp:197
 
+static llvm::cl::opt<bool> IncludeFixIts(
+    "include-fixits",
----------------
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?


================
Comment at: clangd/tool/ClangdMain.cpp:198
+static llvm::cl::opt<bool> IncludeFixIts(
+    "include-fixits",
+    llvm::cl::desc(
----------------
ilya-biryukov wrote:
> This is a bit too generic name for the binary. Maybe `-completion-fixits`?
clang calls changes attached to diagnostics "fix-it hints", clangd calls them "fixes".

This isn't either of those, maybe it should have a different name?
Or if clang is going to consider them as another type of fixit, we should probably call them fixes.
Maybe `-completions-with-fixes`, which hints that if the flag is off you're going to lose these completions entirely?


================
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:
> 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.


================
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:
> 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?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51214





More information about the cfe-commits mailing list