[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rL362939: [clangd] Revamp textDocument/onTypeFormatting. (authored by sammccall, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-comm

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall marked 3 inline comments as done. sammccall added a comment. OK, this has been stuck for a while and (as discussed a bit offline) I haven't been able to make the alternative approaches work. In D60605#1495268 , @yvvan wrote: > @ilya-biryukov >

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few NITs and a few questions about the possibility of moving this to `clang-format` at some point in the future. From what I can to simplify the calling, we need to:

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. In D60605#1495268 , @yvvan wrote: > @ilya-biryukov > What do you think about D53072 ? It can be > polished and combined with this change removing some code from here (which I > assume is a

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

yvvan added a comment. @ilya-biryukov What do you think about D53072 ? It can be polished and combined with this change removing some code from here (which I assume is a good thing). The idea there is that clang-format knows that it's not allowed to remove new

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Sorry for the delay, I'll make sure to take a close look at the change tomorrow. As mentioned in offline discussions, doing this through `clang-format` seems like the right approach. At the same the markers we put into the files to drive formatting seem fragile, g

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall added a comment. In D60605#1488039 , @ilya-biryukov wrote: > Maybe that's due to some extra logic applied in VSCode on top of what we do. > Let me know if this does not reproduce. Aha, the missing piece was that vscode reindented the cursor t

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall updated this revision to Diff 197928. sammccall added a comment. Fix latest case (trailing comment outdented in vscode). Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605 Files: clangd/CMakeLists.t

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. In D60605#1485375 , @sammccall wrote: > I'm not able to reproduce this, can you give a complete example? After adding a newline here: int test(bool x) { if (x) return 10; // All spelled tokens are accounted for

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall added a comment. In D60605#1483729 , @ilya-biryukov wrote: > Found today: I'm not able to reproduce this, can you give a complete example? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Found today: if (something) return; // some long comment // that takes two lines.^ Expected: comment on last line is not re-indented. Actual (comment re-indented): if (something) return; // some long comment // that takes two lines. Rep

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall updated this revision to Diff 196864. sammccall added a comment. Fix more comment contination cases. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605 Files: clangd/CMakeLists.txt clangd/ClangdLS

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Input: if (something) foo; // a comment^ Expected: if (something) foo; // a comment ^ Actual (indented to align with a comment): if (something) foo; // a comment ^ Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION h

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Input: // this is my comment. ^ // and it continues on the next line. Expected: // this is my comment. // ^ // and it continues on the next line. Actual: // this is my comment. ^ // and it continues on the next line. Repository: rCTE Clang To

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. In D60605#1478922 , @sammccall wrote: > In D60605#1478581 , @ilya-biryukov > wrote: > > > Input: > > > > int test() { > > }^ > > > > > > Actual: > > > > int test() {} > > ^ >

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall added a comment. In D60605#1478579 , @ilya-biryukov wrote: > Another example: > > int test() { > > ^ > } > > > Expected: a newline was added. > Actual: newline does not allow to be added. Fixed. In D60605#1478581

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall updated this revision to Diff 196656. sammccall added a comment. Avoid removing lines when formatting after \n. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605 Files: clangd/CMakeLists.txt clan

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Input: int test() { }^ Expected: int test() { } ^ Actual: int test() {} ^ Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605 _

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Another example: int test() { ^ } Expected: a newline was added. Actual: newline does not allow to be added. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall updated this revision to Diff 196538. sammccall added a comment. Fix comment wrapping behavior: - when splitting a comment before //, don't add another // - if the editor inserts // before the cursor to continue a line comment, indent it and adjust to three slashes if needed. This doe

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Found an issue today. Input (break a line at ^): // here is my comment ^// another comment Expected: // here is my comment ^// another comment Actual (an extra comment marker is added): // here is my comment // // another comment Repository: rCTE

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall added a comment. @kadircet if you're interested in the behavior here, you can patch this in and try out with vscode. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605 _

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall updated this revision to Diff 195338. sammccall added a comment. Lots more test cases and better handling of braced blocks. This turns out to be an interesting case: if (foo) { ^} Really the right thing to do here is insert another newline. Unfortuantely that means inserting text

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall updated this revision to Diff 194855. sammccall added a comment. Unit tests. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/Cla

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

sammccall created this revision. sammccall added reviewers: ilya-biryukov, hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, mgorny. Herald added a project: clang. The existing implementation (which triggers on }) is fairly simple and has flaws: - doesn