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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits