[PATCH] D57739: [clangd] Format tweak's replacements.

hokein added a comment. In D57739#1392453 , @sammccall wrote: > In D57739#1390374 , @ilya-biryukov > wrote: > > > In D57739#1390321 , @sammccall > > wrote: > > > > > It's

[PATCH] D57739: [clangd] Format tweak's replacements.

sammccall added a comment. In D57739#1390374 , @ilya-biryukov wrote: > In D57739#1390321 , @sammccall wrote: > > > It's not about stability or whether the functionality is desired, but > > layering. > > Unit

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov added a comment. In D57739#1390321 , @sammccall wrote: > It's not about stability or whether the functionality is desired, but > layering. > Unit tests having narrow scope is a good thing - if we want system tests > that check

[PATCH] D57739: [clangd] Format tweak's replacements.

sammccall added a comment. In D57739#1390315 , @ilya-biryukov wrote: > In D57739#1390252 , @sammccall wrote: > > > I agree with this concern, and don't think this is an appropriate layer to > > be aware of

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov added a comment. In D57739#1390252 , @sammccall wrote: > I agree with this concern, and don't think this is an appropriate layer to be > aware of styling or failing on format errors. Can we consider moving the > formatting to clangdserver

[PATCH] D57739: [clangd] Format tweak's replacements.

sammccall added a comment. In D57739#1385144 , @hokein wrote: > In D57739#1384844 , @ilya-biryukov > wrote: > > > Could we move the code to the `Tweak` itself, so that all the callers would > > get the

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein closed this revision. hokein added a comment. Committed in rL353306 . Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57739/new/ https://reviews.llvm.org/D57739

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein updated this revision to Diff 185551. hokein added a comment. Remove accident change. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57739/new/ https://reviews.llvm.org/D57739 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein updated this revision to Diff 185550. hokein marked an inline comment as done. hokein added a comment. Address comments. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57739/new/ https://reviews.llvm.org/D57739 Files:

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM! Also a few NITs. Comment at: clangd/ClangdServer.cpp:155 Opts.ClangTidyOpts = ClangTidyOptProvider->getOptions(File); + // FIXME: cache this +

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein added inline comments. Comment at: clangd/refactor/Tweak.h:56 +/// The style to format generated changes. +format::FormatStyle Style; }; ilya-biryukov wrote: > NIT: Maybe make this a second argument of `apply`? > This would convey the idea that

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein updated this revision to Diff 185530. hokein marked 3 inline comments as done. hokein added a comment. Address review comments. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57739/new/ https://reviews.llvm.org/D57739 Files:

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein updated this revision to Diff 185531. hokein added a comment. Add missing file. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57739/new/ https://reviews.llvm.org/D57739 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov added inline comments. Comment at: clangd/refactor/Tweak.h:81 + /// EXPECTS: prepare() was called and returned true. + virtual Expected execute(const Selection ) = 0; }; hokein wrote: > I think the current name is slightly better than `doApply`,

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov added inline comments. Comment at: clangd/refactor/Tweak.h:56 +/// The style to format generated changes. +format::FormatStyle Style; }; NIT: Maybe make this a second argument of `apply`? This would convey the idea that `execute()`

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein marked an inline comment as done. hokein added inline comments. Comment at: clangd/refactor/Tweak.h:81 + /// EXPECTS: prepare() was called and returned true. + virtual Expected execute(const Selection ) = 0; }; I think the current name is slightly

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein updated this revision to Diff 185529. hokein added a comment. Move format to the tweak. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57739/new/ https://reviews.llvm.org/D57739 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov added a comment. In D57739#1385144 , @hokein wrote: > This seems introduce intrusive changes to the Tweak interface (Tweak will > need to know the `FormatStyle` somehow), I think this might be done in > another patch. It's totally fine,

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein added a comment. In D57739#1384844 , @ilya-biryukov wrote: > Could we move the code to the `Tweak` itself, so that all the callers would > get the formatting? I.e. > > class Tweak { > Replacements apply() { // This is now non-virtual. >

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein updated this revision to Diff 185306. hokein marked 9 inline comments as done. hokein added a comment. Adress review comments. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57739/new/ https://reviews.llvm.org/D57739 Files:

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:366 + auto Style = getFormatStyle(Code, File); + if (!Style) +return; ioeric wrote: > hokein wrote: > > not sure the err-handling strategy here -- maybe if this is failed, we > >

[PATCH] D57739: [clangd] Format tweak's replacements.

ilya-biryukov added a comment. Could we move the code to the `Tweak` itself, so that all the callers would get the formatting? I.e. class Tweak { Replacements apply() { // This is now non-virtual. auto Replacements = doApply(); return cleanupAndFormat(Replacements); }

[PATCH] D57739: [clangd] Format tweak's replacements.

ioeric added inline comments. Comment at: clangd/ClangdServer.cpp:366 + auto Style = getFormatStyle(Code, File); + if (!Style) +return; hokein wrote: > not sure the err-handling strategy here -- maybe if this is failed, we still > apply replacements

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein marked an inline comment as done. hokein added inline comments. Comment at: clangd/ClangdServer.cpp:366 + auto Style = getFormatStyle(Code, File); + if (!Style) +return; not sure the err-handling strategy here -- maybe if this is failed, we still

[PATCH] D57739: [clangd] Format tweak's replacements.

hokein created this revision. hokein added a reviewer: ilya-biryukov. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric. Herald added a project: clang. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D57739 Files: clangd/ClangdLSPServer.cpp