[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:
> > > sammccall wrote:
> > > > kadircet wrote:
> > > > > 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. 
> > > > Do we have any more details here? I'm still skeptical that exposing this to end users will help at all, this seems likely that it should be a capability if we do need it.
> > > No updates  on the issue. Here it is:
> > > https://github.com/Microsoft/language-server-protocol/issues/543
> > > 
> > > Not sure capability is the right thing there, the problem is that additionalTextEdits are underspecified and implemented differently in every client. What we need is a fix in the protocol and fixes in all the clients.
> > > 
> > > Sadly, this only works in YCM-based completer for now (of all we tested)
> > Sure, sounds like protocol fix is the long-term answer. I don't think adding user-facing options are better than nothing. If YCM does the right thing and we want to disable it for everyone not on YCM, we can add a `textEditsAreAppliedInOrder` capability to the YCM completer and treat that as an opt-in. It's not clear what the advantage of a user-facing flag over an editor-facing capability is for this purpose.
> > 
> > Mostly given LSP is unclear here it seems this feature isn't ready for prime-time.
> > Could we fix it on our side by coalescing multiple edits into a single one?
> I agree, the feature is not very useful if it breaks everywhere. Removing the option and exploring other ways to do it LG.
> 
> > Could we fix it on our side by coalescing multiple edits into a single one?
> We tried to combine additionalTextEdits into the main textEdit, that's what works in YCM.
> However, it did not help in other editors, they misinterpret a main textedit (each in a different way) if it affects anything before the start of the completion identifier, which is exactly the case for the only fix we have at the time, that is `. to ->`.
Deleting this option and waiting until editors out there start to handle additionalTextEdits in a similar way.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51214