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

[Sam McCall via Phabricator via cfe-commits]

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-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60605?vs=197928=203821#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605

Files:
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/Format.cpp
  clang-tools-extra/trunk/clangd/Format.h
  clang-tools-extra/trunk/clangd/test/formatting.test
  clang-tools-extra/trunk/clangd/test/initialize-params.test
  clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
  clang-tools-extra/trunk/clangd/unittests/FormatTests.cpp

Index: clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
@@ -39,6 +39,7 @@
   FileIndexTests.cpp
   FindSymbolsTests.cpp
   FormattedStringTests.cpp
+  FormatTests.cpp
   FSTests.cpp
   FunctionTests.cpp
   FuzzyMatchTests.cpp
Index: clang-tools-extra/trunk/clangd/unittests/FormatTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/FormatTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/FormatTests.cpp
@@ -0,0 +1,308 @@
+//===-- FormatTests.cpp - Automatic code formatting tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Format.h"
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestFS.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string afterTyped(llvm::StringRef CodeWithCursor,
+   llvm::StringRef Typed) {
+  Annotations Code(CodeWithCursor);
+  unsigned Cursor = llvm::cantFail(positionToOffset(Code.code(), Code.point()));
+  auto Changes =
+  formatIncremental(Code.code(), Cursor, Typed,
+format::getGoogleStyle(format::FormatStyle::LK_Cpp));
+  tooling::Replacements Merged;
+  for (const auto& R : Changes)
+if (llvm::Error E = Merged.add(R))
+  ADD_FAILURE() << llvm::toString(std::move(E));
+  auto NewCode = tooling::applyAllReplacements(Code.code(), Merged);
+  EXPECT_TRUE(bool(NewCode))
+  << "Bad replacements: " << llvm::toString(NewCode.takeError());
+  NewCode->insert(transformCursorPosition(Cursor, Changes), "^");
+  return *NewCode;
+}
+
+// We can't pass raw strings directly to EXPECT_EQ because of gcc bugs.
+void expectAfterNewline(const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, "\n")) << Before;
+}
+void expectAfter(const char *Typed, const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, Typed)) << Before;
+}
+
+TEST(FormatIncremental, SplitComment) {
+  expectAfterNewline(R"cpp(
+// this comment was
+^split
+)cpp",
+   R"cpp(
+// this comment was
+// ^split
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// trailing whitespace is not a split
+^   
+)cpp",
+   R"cpp(
+// trailing whitespace is not a split
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// splitting a
+^
+// multiline comment
+)cpp",
+ R"cpp(
+// splitting a
+// ^
+// multiline comment
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// extra   
+^ whitespace
+)cpp",
+   R"cpp(
+// extra
+// ^whitespace
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// triple
+^slash
+)cpp",
+   R"cpp(
+/// triple
+/// ^slash
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// editor continuation
+//^
+)cpp",
+   R"cpp(
+/// editor continuation
+/// ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// break before
+^ // slashes
+)cpp",
+   R"cpp(
+// break before
+^// slashes
+)cpp");
+
+
+  expectAfterNewline(R"cpp(
+int x;  // aligned
+^comment
+)cpp",
+   R"cpp(
+int x;  // aligned
+// ^comment
+)cpp");
+
+  // Fixed bug: the second line of the aligned comment shouldn't be "attached"
+  // to the cursor and outdented.
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (x)
+return; // All spelled tokens are accounted for.
+// that takes two lines
+^
+}
+)cpp",
+   R"cpp(
+void foo() {
+  if (x)
+return;  // All spelled tokens are accounted for.

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

[Sam McCall via Phabricator via cfe-commits]

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 
>  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 lines and it always marks them with MustBreakBefore.
>  I'm not sure if anybody needs an executable but I think it's not a big deal 
> to have an extra reformat() function.
>
> It is also safe because it was the only way I found which does not break the 
> code style like, for instance, adding comment block to the end of the 
> previous line.


I'm not sure D53072  helps in its current 
form. Preserving newlines with content on the line isn't always the (IMO) right 
formatting: see e.g. FormatBrace test.
(I have other concerns about the details of that patch, but they don't really 
relate to this one).

In D60605#1518031 , @ilya-biryukov 
wrote:

> 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:
>
> 1. teach `clang-format` about a cursor (by making it a special token or 
> somehow else), this would allow to avoid some book-keeping;


There's at least one wrinkle with making it a special token: it may be within a 
token. E.g. after splitting a line comment, the cursor is at offset 3 within 
`// the new line`. Not sure how deep this problem goes.

> 2. teach `clang-format` to format in presence of unmatched parentheses. 
> Again, would simplify things. Although not sure it's easy to do in 
> `clang-format` either;

Note this needs to be a style or other option, as the fact that clang-format 
bails out on broken code in general is well-established desired behavior.
I attempted this (by having the parser return "true" rather than "false" on 
reaching the end of a paren/brace list, and got lost in the details. I'm not 
sure whether it's likely to work, or whether clang-format would just do the 
same source transformation as we do here. (Obviously sharing it would be nice).

> 3. teach `clang-format` to respect indentation before the cursor (added by 
> the editor in our case).

I'm not sure what you mean by this: I don't think we want that indentation to 
be respected, but rather reformatted away.

> After that we don't seem to need the complicated multi-pass replacements.

We currently have pre-formatting changes (e.g. comment splitting). I'm not sure 
if you expect these to belong to clang-format, but I wouldn't. (They're more 
like a "respond to newline" helper for editors than they are like formatting a 
range). So you'd go from {pre-formatting changes, insert placeholder, 
clang-format, remove placeholder} to {pre-formatting changes, clang-format}.
So the logic to implement multi-pass replacements is still needed I think, 
though you'd have fewer actual passes.

> - Am I missing something that we also need from `clang-format` here?

The comment splitting and {} splitting, I think.

> - How would you estimate the amount of work needed to move this functionality 
> to `clang-format`?
> - In general, do you think it's worth moving it there or not?

I don't know, I put maybe 8 hours into this approach and didn't make any 
progress.
If someone finds a clean way to do it, I'd be in favour. But a 
hacky/brute-force way would do more harm than good in my opinion.

> In the meantime, I've found another case where newline gets eaten:

Yes :-(
I've added a test to document this bad behavior. It's very hard to fix with 
clang-format as a black-box, because it hard-codes erasure of blank lines 
adjacent to a formatted region.
Probably the simplest approach is to add a clang-format option to always 
preserve blank lines. Although the motivation isn't an actual style guide, it 
makes sense to think of this as a style rule, it's very simple, and it's 
related to existing options.




Comment at: clangd/Format.h:25
+/// Applies limited formatting around new \p InsertedText.
+/// The \p Code already contains the updated text near \p Cursor, and may have
+/// had additional / characters (such as indentation) inserted by the editor.

ilya-biryukov wrote:
> NIT: maybe be more concrete? `near \p Cursor` means `right before \p Cursor`? 
> Or can it have other characters (e.g. whitespace in your example)?
s/near/before/.
Yes, it can have other characters, these are already mentioned in the comment.


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 via Phabricator via cfe-commits]

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:

1. teach `clang-format` about a cursor (by making it a special token or somehow 
else), this would allow to avoid some book-keeping;
2. teach `clang-format` to format in presence of unmatched parentheses. Again, 
would simplify things. Although not sure it's easy to do in `clang-format` 
either;
3. teach `clang-format` to respect indentation before the cursor (added by the 
editor in our case).

After that we don't seem to need the complicated multi-pass replacements.

- Am I missing something that we also need from `clang-format` here?
- How would you estimate the amount of work needed to move this functionality 
to `clang-format`?
- In general, do you think it's worth moving it there or not?




Comment at: clangd/Format.cpp:83
+
+llvm::StringRef Filename = "";
+

NIT:  put a comment about filename being required but ignored here to explain 
why we need this variable.



Comment at: clangd/Format.h:25
+/// Applies limited formatting around new \p InsertedText.
+/// The \p Code already contains the updated text near \p Cursor, and may have
+/// had additional / characters (such as indentation) inserted by the editor.

NIT: maybe be more concrete? `near \p Cursor` means `right before \p Cursor`? 
Or can it have other characters (e.g. whitespace in your example)?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 good thing).
>  The idea there is that clang-format knows that it's not allowed to remove 
> new lines and it always marks them with MustBreakBefore.
>  I'm not sure if anybody needs an executable but I think it's not a big deal 
> to have an extra reformat() function.
>
> It is also safe because it was the only way I found which does not break the 
> code style like, for instance, adding comment block to the end of the 
> previous line.


I also think (both Sam and you seem to agree with this) that we'll definitely 
need changes to `clang-format` to support this use-case.
Also sympathetic to the view that this change should probably live in 
`clang-format`, but having it in clangd first and moving to `clang-format` 
later also LG.
@sammccall, what's your plan there? Experimenting in `clangd` and moving to 
`clang-format` later? Any reason to not start in `clang-format` in the first 
place?

Will add a few comments regarding the need for a separate tool in D53072 
 directly.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

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 
lines and it always marks them with MustBreakBefore.
I'm not sure if anybody needs an executable but I think it's not a big deal to 
have an extra reformat() function.

It is also safe because it was the only way I found which does not break the 
code style like, for instance, adding comment block to the end of the previous 
line.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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, giving results we don't want.
The biggest issue that I can see is that it's super-hard to predict what 
`clang-format` is going to do in each case and even harder to certify that this 
should work in general case.

Intuitively, moving the logic to `clang-format` seems like the right thing to 
do in the long run (e.g. introducing a special cursor marker into clang-format, 
similar a code completion marker used by clang or something similar).
OTOH, it's hard for me to asses the amount of work needed to do this inside 
`clang-format` itself (not a `clang-format` expert), and the patch is 
definitely an improvement to what we had before (I've been using it for awhile 
now). The problems do get fixed, so I'm totally happy with it landing as is if 
we commit to fixing those nasty cases until we run out of them.

PS. In the meantime, I've found another case where newline gets eaten:

  class SyntaxTreeTest {
  public:
^ 
  };

**Expected:** newline is added.
**Actual:** does not let to add a newline (the added newline is removed).


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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 to align with the 
comment (but didn't continue the comment).
This caused us to replace the cursor with an *indented* identifier, and 
clang-format thought the last line of the comment was attached to it.

It almost seems silly to keep patching the code rather than add a "break here" 
option to clang-format that doesn't have side-effects. However the fix here is 
pretty simple, so why not.

Do you think this is converging to something we should land? The last few have 
been pretty small tweaks and I'm happy to keep fixing those as incremental 
patches.
Conversely, if the basic design isn't right, then further polish probably isn't 
worthwhile.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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.txt
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.cpp
  clangd/Format.h
  test/clangd/formatting.test
  test/clangd/initialize-params.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/FormatTests.cpp

Index: unittests/clangd/FormatTests.cpp
===
--- /dev/null
+++ unittests/clangd/FormatTests.cpp
@@ -0,0 +1,294 @@
+//===-- FormatTests.cpp - Automatic code formatting tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Format.h"
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestFS.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string afterTyped(llvm::StringRef CodeWithCursor,
+   llvm::StringRef Typed) {
+  Annotations Code(CodeWithCursor);
+  unsigned Cursor = llvm::cantFail(positionToOffset(Code.code(), Code.point()));
+  auto Changes =
+  formatIncremental(Code.code(), Cursor, Typed,
+format::getGoogleStyle(format::FormatStyle::LK_Cpp));
+  tooling::Replacements Merged;
+  for (const auto& R : Changes)
+if (llvm::Error E = Merged.add(R))
+  ADD_FAILURE() << llvm::toString(std::move(E));
+  auto NewCode = tooling::applyAllReplacements(Code.code(), Merged);
+  EXPECT_TRUE(bool(NewCode))
+  << "Bad replacements: " << llvm::toString(NewCode.takeError());
+  NewCode->insert(transformCursorPosition(Cursor, Changes), "^");
+  return *NewCode;
+}
+
+// We can't pass raw strings directly to EXPECT_EQ because of gcc bugs.
+void expectAfterNewline(const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, "\n")) << Before;
+}
+void expectAfter(const char *Typed, const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, Typed)) << Before;
+}
+
+TEST(FormatIncremental, SplitComment) {
+  expectAfterNewline(R"cpp(
+// this comment was
+^split
+)cpp",
+   R"cpp(
+// this comment was
+// ^split
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// trailing whitespace is not a split
+^   
+)cpp",
+   R"cpp(
+// trailing whitespace is not a split
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// splitting a
+^
+// multiline comment
+)cpp",
+ R"cpp(
+// splitting a
+// ^
+// multiline comment
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// extra   
+^ whitespace
+)cpp",
+   R"cpp(
+// extra
+// ^whitespace
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// triple
+^slash
+)cpp",
+   R"cpp(
+/// triple
+/// ^slash
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// editor continuation
+//^
+)cpp",
+   R"cpp(
+/// editor continuation
+/// ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// break before
+^ // slashes
+)cpp",
+   R"cpp(
+// break before
+^// slashes
+)cpp");
+
+
+  expectAfterNewline(R"cpp(
+int x;  // aligned
+^comment
+)cpp",
+   R"cpp(
+int x;  // aligned
+// ^comment
+)cpp");
+
+  // Fixed bug: the second line of the aligned comment shouldn't be "attached"
+  // to the cursor and outdented.
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (x)
+return; // All spelled tokens are accounted for.
+// that takes two lines
+^
+}
+)cpp",
+   R"cpp(
+void foo() {
+  if (x)
+return;  // All spelled tokens are accounted for.
+ // that takes two lines
+  ^
+}
+)cpp");
+}
+
+TEST(FormatIncremental, Indentation) {
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp",
+   R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+  bar(baz(
+^
+)cpp",
+   R"cpp(
+void foo() {
+  bar(baz(
+  ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+^}
+)cpp",
+   R"cpp(
+void foo() {
+  ^
+}
+)cpp");
+
+  expectAfterNewline(R"cpp(
+class X {
+protected:
+^
+)cpp",
+   R"cpp(
+class X {
+ protected:
+  ^
+)cpp");
+
+// Mismatched brackets (1)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^}
+}
+)cpp");
+// Mismatched brackets (2)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^text}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^text}
+}
+)cpp");
+// Matched brackets
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^)
+}
+)cpp",
+   

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

[Ilya Biryukov via Phabricator via cfe-commits]

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.
 // that takes two lines^
  }

I get:

  int test(bool x) {
if (x)
  return 10; // All spelled tokens are accounted for.
// that takes two lines
^
  }

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.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.cpp
  clangd/Format.h
  test/clangd/formatting.test
  test/clangd/initialize-params.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/FormatTests.cpp

Index: unittests/clangd/FormatTests.cpp
===
--- /dev/null
+++ unittests/clangd/FormatTests.cpp
@@ -0,0 +1,275 @@
+//===-- FormatTests.cpp - Automatic code formatting tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Format.h"
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestFS.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string afterTyped(llvm::StringRef CodeWithCursor,
+   llvm::StringRef Typed) {
+  Annotations Code(CodeWithCursor);
+  unsigned Cursor = llvm::cantFail(positionToOffset(Code.code(), Code.point()));
+  auto Changes =
+  formatIncremental(Code.code(), Cursor, Typed,
+format::getGoogleStyle(format::FormatStyle::LK_Cpp));
+  tooling::Replacements Merged;
+  for (const auto& R : Changes)
+if (llvm::Error E = Merged.add(R))
+  ADD_FAILURE() << llvm::toString(std::move(E));
+  auto NewCode = tooling::applyAllReplacements(Code.code(), Merged);
+  EXPECT_TRUE(bool(NewCode))
+  << "Bad replacements: " << llvm::toString(NewCode.takeError());
+  NewCode->insert(transformCursorPosition(Cursor, Changes), "^");
+  return *NewCode;
+}
+
+// We can't pass raw strings directly to EXPECT_EQ because of gcc bugs.
+void expectAfterNewline(const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, "\n")) << Before;
+}
+void expectAfter(const char *Typed, const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, Typed)) << Before;
+}
+
+TEST(FormatIncremental, SplitComment) {
+  expectAfterNewline(R"cpp(
+// this comment was
+^split
+)cpp",
+   R"cpp(
+// this comment was
+// ^split
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// trailing whitespace is not a split
+^   
+)cpp",
+   R"cpp(
+// trailing whitespace is not a split
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// splitting a
+^
+// multiline comment
+)cpp",
+ R"cpp(
+// splitting a
+// ^
+// multiline comment
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// extra   
+^ whitespace
+)cpp",
+   R"cpp(
+// extra
+// ^whitespace
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// triple
+^slash
+)cpp",
+   R"cpp(
+/// triple
+/// ^slash
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// editor continuation
+//^
+)cpp",
+   R"cpp(
+/// editor continuation
+/// ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// break before
+^ // slashes
+)cpp",
+   R"cpp(
+// break before
+^// slashes
+)cpp");
+
+
+  expectAfterNewline(R"cpp(
+int x;  // aligned
+^comment
+)cpp",
+   R"cpp(
+int x;  // aligned
+// ^comment
+)cpp");
+}
+
+TEST(FormatIncremental, Indentation) {
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp",
+   R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+  bar(baz(
+^
+)cpp",
+   R"cpp(
+void foo() {
+  bar(baz(
+  ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+^}
+)cpp",
+   R"cpp(
+void foo() {
+  ^
+}
+)cpp");
+
+  expectAfterNewline(R"cpp(
+class X {
+protected:
+^
+)cpp",
+   R"cpp(
+class X {
+ protected:
+  ^
+)cpp");
+
+// Mismatched brackets (1)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^}
+}
+)cpp");
+// Mismatched brackets (2)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^text}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^text}
+}
+)cpp");
+// Matched brackets
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^)
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^)
+}
+)cpp");
+}
+
+TEST(FormatIncremental, FormatPreviousLine) {
+  expectAfterNewline(R"cpp(
+void foo() {
+   untouched( );
+int x=2;
+^
+)cpp",
+ R"cpp(
+void foo() {
+   untouched( );
+   int x = 2;
+   ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+int x=untouched( );
+auto L = []{return;return;};
+^
+)cpp",
+   R"cpp(
+int x=untouched( );
+auto L = [] {
+  return;
+  return;

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

[Ilya Biryukov via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D60605#1478922 , @sammccall wrote:

> In D60605#1478581 , @ilya-biryukov 
> wrote:
>
> > Input:
> >
> >   int test() {
> >   }^
> >
> >
> > Actual:
> >
> >   int test() {}
> >   ^
> >
>
>
> I really do think this is the right thing in particular if you typed the 
> brace.
>  It's visually confusing that hitting \n results in the cursor staying on the 
> same line and the } moving.
>  And this interacts badly with the previous bug.
>  But can you see if this is something you could live with, and if not, 
> explain why you don't want the {} formatted?


In general, I have high tolerance for quirks in IDE features. But this one 
really makes me itchy. I'd say clangd should never remove lines while I'm 
trying to add them. Are there examples where this behavior is useful?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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 , @ilya-biryukov 
wrote:

> Input:
>
>   int test() {
>   }^
>
>
> Actual:
>
>   int test() {}
>   ^
>


I really do think this is the right thing in particular if you typed the brace.
It's visually confusing that hitting \n results in the cursor staying on the 
same line and the } moving.
And this interacts badly with the previous bug.
But can you see if this is something you could live with, and if not, explain 
why you don't want the {} formatted?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.cpp
  clangd/Format.h
  test/clangd/formatting.test
  test/clangd/initialize-params.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/FormatTests.cpp

Index: unittests/clangd/FormatTests.cpp
===
--- /dev/null
+++ unittests/clangd/FormatTests.cpp
@@ -0,0 +1,264 @@
+//===-- FormatTests.cpp - Automatic code formatting tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Format.h"
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestFS.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string afterTyped(llvm::StringRef CodeWithCursor,
+   llvm::StringRef Typed) {
+  Annotations Code(CodeWithCursor);
+  unsigned Cursor = llvm::cantFail(positionToOffset(Code.code(), Code.point()));
+  auto Changes =
+  formatIncremental(Code.code(), Cursor, Typed,
+format::getGoogleStyle(format::FormatStyle::LK_Cpp));
+  tooling::Replacements Merged;
+  for (const auto& R : Changes)
+if (llvm::Error E = Merged.add(R))
+  ADD_FAILURE() << llvm::toString(std::move(E));
+  auto NewCode = tooling::applyAllReplacements(Code.code(), Merged);
+  EXPECT_TRUE(bool(NewCode))
+  << "Bad replacements: " << llvm::toString(NewCode.takeError());
+  NewCode->insert(transformCursorPosition(Cursor, Changes), "^");
+  return *NewCode;
+}
+
+// We can't pass raw strings directly to EXPECT_EQ because of gcc bugs.
+void expectAfterNewline(const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, "\n")) << Before;
+}
+void expectAfter(const char *Typed, const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, Typed)) << Before;
+}
+
+TEST(FormatIncremental, SplitComment) {
+  expectAfterNewline(R"cpp(
+// this comment was
+^split
+)cpp",
+   R"cpp(
+// this comment was
+// ^split
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// trailing whitespace is not a split
+^   
+)cpp",
+   R"cpp(
+// trailing whitespace is not a split
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// extra   
+^ whitespace
+)cpp",
+   R"cpp(
+// extra
+// ^whitespace
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// triple
+^slash
+)cpp",
+   R"cpp(
+/// triple
+/// ^slash
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// editor continuation
+//^
+)cpp",
+   R"cpp(
+/// editor continuation
+/// ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// break before
+^ // slashes
+)cpp",
+   R"cpp(
+// break before
+^// slashes
+)cpp");
+
+
+  expectAfterNewline(R"cpp(
+int x;  // aligned
+^comment
+)cpp",
+   R"cpp(
+int x;  // aligned
+// ^comment
+)cpp");
+}
+
+TEST(FormatIncremental, Indentation) {
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp",
+   R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+  bar(baz(
+^
+)cpp",
+   R"cpp(
+void foo() {
+  bar(baz(
+  ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+^}
+)cpp",
+   R"cpp(
+void foo() {
+  ^
+}
+)cpp");
+
+  expectAfterNewline(R"cpp(
+class X {
+protected:
+^
+)cpp",
+   R"cpp(
+class X {
+ protected:
+  ^
+)cpp");
+
+// Mismatched brackets (1)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^}
+}
+)cpp");
+// Mismatched brackets (2)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^text}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^text}
+}
+)cpp");
+// Matched brackets
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^)
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^)
+}
+)cpp");
+}
+
+TEST(FormatIncremental, FormatPreviousLine) {
+  expectAfterNewline(R"cpp(
+void foo() {
+   untouched( );
+int x=2;
+^
+)cpp",
+ R"cpp(
+void foo() {
+   untouched( );
+   int x = 2;
+   ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+int x=untouched( );
+auto L = []{return;return;};
+^
+)cpp",
+   R"cpp(
+int x=untouched( );
+auto L = [] {
+  return;
+  return;
+};
+^
+)cpp");
+}
+
+TEST(FormatIncremental, Annoyances) {
+  // Don't remove newlines the user typed!
+  expectAfterNewline(R"cpp(
+int x(){
+
+
+^
+}

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

[Ilya Biryukov via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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 doesn't mean enter at the *end* of a comment will continue the comment,
but if editors do that (vim does) then it will be respected.


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/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.cpp
  clangd/Format.h
  test/clangd/formatting.test
  test/clangd/initialize-params.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/FormatTests.cpp

Index: unittests/clangd/FormatTests.cpp
===
--- /dev/null
+++ unittests/clangd/FormatTests.cpp
@@ -0,0 +1,246 @@
+//===-- FormatTests.cpp - Automatic code formatting tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Format.h"
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestFS.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string afterTyped(llvm::StringRef CodeWithCursor,
+   llvm::StringRef Typed) {
+  Annotations Code(CodeWithCursor);
+  unsigned Cursor = llvm::cantFail(positionToOffset(Code.code(), Code.point()));
+  auto Changes =
+  formatIncremental(Code.code(), Cursor, Typed,
+format::getGoogleStyle(format::FormatStyle::LK_Cpp));
+  tooling::Replacements Merged;
+  for (const auto& R : Changes)
+if (llvm::Error E = Merged.add(R))
+  ADD_FAILURE() << llvm::toString(std::move(E));
+  auto NewCode = tooling::applyAllReplacements(Code.code(), Merged);
+  EXPECT_TRUE(bool(NewCode))
+  << "Bad replacements: " << llvm::toString(NewCode.takeError());
+  NewCode->insert(transformCursorPosition(Cursor, Changes), "^");
+  return *NewCode;
+}
+
+// We can't pass raw strings directly to EXPECT_EQ because of gcc bugs.
+void expectAfterNewline(const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, "\n")) << Before;
+}
+void expectAfter(const char *Typed, const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, Typed)) << Before;
+}
+
+TEST(FormatIncremental, SplitComment) {
+  expectAfterNewline(R"cpp(
+// this comment was
+^split
+)cpp",
+   R"cpp(
+// this comment was
+// ^split
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// trailing whitespace is not a split
+^   
+)cpp",
+   R"cpp(
+// trailing whitespace is not a split
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// extra   
+^ whitespace
+)cpp",
+   R"cpp(
+// extra
+// ^whitespace
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// triple
+^slash
+)cpp",
+   R"cpp(
+/// triple
+/// ^slash
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// editor continuation
+//^
+)cpp",
+   R"cpp(
+/// editor continuation
+/// ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// break before
+^ // slashes
+)cpp",
+   R"cpp(
+// break before
+^// slashes
+)cpp");
+
+
+  expectAfterNewline(R"cpp(
+int x;  // aligned
+^comment
+)cpp",
+   R"cpp(
+int x;  // aligned
+// ^comment
+)cpp");
+}
+
+TEST(FormatIncremental, Indentation) {
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp",
+   R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+  bar(baz(
+^
+)cpp",
+   R"cpp(
+void foo() {
+  bar(baz(
+  ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+^}
+)cpp",
+   R"cpp(
+void foo() {
+  ^
+}
+)cpp");
+
+  expectAfterNewline(R"cpp(
+class X {
+protected:
+^
+)cpp",
+   R"cpp(
+class X {
+ protected:
+  ^
+)cpp");
+
+// Mismatched brackets (1)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^}
+}
+)cpp");
+// Mismatched brackets (2)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^text}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^text}
+}
+)cpp");
+// Matched brackets
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^)
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^)
+}
+)cpp");
+}
+
+TEST(FormatIncremental, FormatPreviousLine) {
+  expectAfterNewline(R"cpp(
+void foo() {
+   untouched( );
+int x=2;
+^
+)cpp",
+ R"cpp(
+void foo() {
+   untouched( );
+   int x = 2;
+   ^
+)cpp");
+
+  

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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 before and after the cursor, which
confuses tooling::Replacements so... lots of churn in this patch :-(


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/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.cpp
  clangd/Format.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/FormatTests.cpp

Index: unittests/clangd/FormatTests.cpp
===
--- /dev/null
+++ unittests/clangd/FormatTests.cpp
@@ -0,0 +1,228 @@
+//===-- FormatTests.cpp - Automatic code formatting tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Format.h"
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestFS.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string afterTyped(llvm::StringRef CodeWithCursor,
+   llvm::StringRef Typed) {
+  Annotations Code(CodeWithCursor);
+  unsigned Cursor = llvm::cantFail(positionToOffset(Code.code(), Code.point()));
+  auto Changes =
+  formatIncremental(Code.code(), Cursor, Typed,
+format::getGoogleStyle(format::FormatStyle::LK_Cpp));
+  tooling::Replacements Merged;
+  for (const auto& R : Changes)
+if (llvm::Error E = Merged.add(R))
+  ADD_FAILURE() << llvm::toString(std::move(E));
+  auto NewCode = tooling::applyAllReplacements(Code.code(), Merged);
+  EXPECT_TRUE(bool(NewCode))
+  << "Bad replacements: " << llvm::toString(NewCode.takeError());
+  NewCode->insert(transformCursorPosition(Cursor, Changes), "^");
+  return *NewCode;
+}
+
+// We can't pass raw strings directly to EXPECT_EQ because of gcc bugs.
+void expectAfterNewline(const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, "\n")) << Before;
+}
+void expectAfter(const char *Typed, const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, Typed)) << Before;
+}
+
+TEST(FormatIncremental, SplitComment) {
+  expectAfterNewline(R"cpp(
+// this comment was
+^split
+)cpp",
+   R"cpp(
+// this comment was
+// ^split
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// trailing whitespace is not a split
+^   
+)cpp",
+   R"cpp(
+// trailing whitespace is not a split
+^
+)cpp");
+
+
+  expectAfterNewline(R"cpp(
+// extra   
+^ whitespace
+)cpp",
+   R"cpp(
+// extra
+// ^whitespace
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// triple
+^slash
+)cpp",
+   R"cpp(
+/// triple
+/// ^slash
+)cpp");
+
+  expectAfterNewline(R"cpp(
+int x;  // aligned
+^comment
+)cpp",
+   R"cpp(
+int x;  // aligned
+// ^comment
+)cpp");
+}
+
+TEST(FormatIncremental, Indentation) {
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp",
+   R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+  bar(baz(
+^
+)cpp",
+   R"cpp(
+void foo() {
+  bar(baz(
+  ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+^}
+)cpp",
+   R"cpp(
+void foo() {
+  ^
+}
+)cpp");
+
+  expectAfterNewline(R"cpp(
+class X {
+protected:
+^
+)cpp",
+   R"cpp(
+class X {
+ protected:
+  ^
+)cpp");
+
+// Mismatched brackets (1)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^}
+}
+)cpp");
+// Mismatched brackets (2)
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^text}
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^text}
+}
+)cpp");
+// Matched brackets
+  expectAfterNewline(R"cpp(
+void foo() {
+  foo{bar(
+^)
+}
+)cpp",
+   R"cpp(
+void foo() {
+  foo {
+bar(
+^)
+}
+)cpp");
+}
+
+TEST(FormatIncremental, FormatPreviousLine) {
+  expectAfterNewline(R"cpp(
+void foo() {
+   untouched( );
+int x=2;
+^
+)cpp",
+ R"cpp(
+void foo() {
+   untouched( );
+   int x = 2;
+   ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+int x=untouched( );
+auto L = []{return;return;};
+^
+)cpp",
+   R"cpp(
+int x=untouched( );
+auto L = [] {
+  return;
+  return;
+};
+^
+)cpp");
+}
+
+TEST(FormatIncremental, FormatBrace) {
+  expectAfter("}", R"cpp(
+vector x= {
+  1,
+  2,
+  3}^
+)cpp",
+  R"cpp(
+vector x = {1, 2, 3}^
+)cpp");

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

[Sam McCall via Phabricator via cfe-commits]

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/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.cpp
  clangd/Format.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/FormatTests.cpp

Index: unittests/clangd/FormatTests.cpp
===
--- /dev/null
+++ unittests/clangd/FormatTests.cpp
@@ -0,0 +1,155 @@
+//===-- FormatTests.cpp - Automatic code formatting tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Format.h"
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestFS.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string afterTyped(llvm::StringRef CodeWithCursor,
+   llvm::StringRef Typed) {
+  Annotations Code(CodeWithCursor);
+  auto Cursor = llvm::cantFail(positionToOffset(Code.code(), Code.point()));
+  auto Changes =
+  formatIncremental(Code.code(), Cursor, Typed,
+format::getGoogleStyle(format::FormatStyle::LK_Cpp));
+  std::string NewCode =
+  cantFail(tooling::applyAllReplacements(Code.code(), Changes));
+  NewCode.insert(Changes.getShiftedCodePosition(Cursor), "^");
+  return NewCode;
+}
+
+// We can't pass raw strings directly to EXPECT_EQ because of gcc bugs.
+void expectAfterNewline(const char *Before, const char *After) {
+  EXPECT_EQ(After, afterTyped(Before, "\n")) << Before;
+}
+
+TEST(FormatIncremental, SplitComment) {
+  expectAfterNewline(R"cpp(
+// this comment was
+^split
+)cpp",
+   R"cpp(
+// this comment was
+// ^split
+)cpp");
+
+  expectAfterNewline(R"cpp(
+// trailing whitespace is not a split
+^   
+)cpp",
+   R"cpp(
+// trailing whitespace is not a split
+^
+)cpp");
+
+
+  expectAfterNewline(R"cpp(
+// extra   
+^ whitespace
+)cpp",
+   R"cpp(
+// extra
+// ^whitespace
+)cpp");
+
+  expectAfterNewline(R"cpp(
+/// triple
+^slash
+)cpp",
+   R"cpp(
+/// triple
+/// ^slash
+)cpp");
+
+  expectAfterNewline(R"cpp(
+int x;  // aligned
+^comment
+)cpp",
+   R"cpp(
+int x;  // aligned
+// ^comment
+)cpp");
+}
+
+TEST(FormatIncremental, Indentation) {
+  expectAfterNewline(R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp",
+   R"cpp(
+void foo() {
+  if (bar)
+^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+void foo() {
+  bar(baz(
+^
+)cpp",
+   R"cpp(
+void foo() {
+  bar(baz(
+  ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+class X {
+protected:
+^
+)cpp",
+   R"cpp(
+class X {
+ protected:
+  ^
+)cpp");
+}
+
+TEST(FormatIncremental, FormatPreviousLine) {
+  expectAfterNewline(R"cpp(
+void foo() {
+   untouched( );
+int x=2;
+^
+)cpp",
+ R"cpp(
+void foo() {
+   untouched( );
+   int x = 2;
+   ^
+)cpp");
+
+  expectAfterNewline(R"cpp(
+int x=untouched( );
+auto L = []{return;return;};
+^
+)cpp",
+   R"cpp(
+int x=untouched( );
+auto L = [] {
+  return;
+  return;
+};
+^
+)cpp");
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -27,6 +27,7 @@
   FileDistanceTests.cpp
   FileIndexTests.cpp
   FindSymbolsTests.cpp
+  FormatTests.cpp
   FSTests.cpp
   FunctionTests.cpp
   FuzzyMatchTests.cpp
Index: clangd/Format.h
===
--- /dev/null
+++ clangd/Format.h
@@ -0,0 +1,45 @@
+//===--- Format.h - automatic code formatting ---*- C++-*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Clangd uses clang-format for formatting operations.
+// This file adapts it to support new scenarios like format-on-type.
+//
+//===--===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FORMAT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FORMAT_H
+
+#include "Protocol.h"
+#include "clang/Format/Format.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace clangd {
+
+/// Applies limited formatting around new \p InsertedText.
+/// The \p Code already 

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

[Sam McCall via Phabricator via cfe-commits]

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't trigger frequently/regularly enough (particularly in editors that 
type the } for you)
- often reformats too much code around the edit
- has jarring cases that I don't have clear ideas for fixing

This implementation is designed to trigger on newline, which feels to me more
intuitive than } or ;.
It does have allow for reformatting after other characters - it has a
basic behavior and a model for adding specialized behavior for
particular characters. But at least initially I'd stick to advertising
\n in the capabilities.

This also handles comment splitting: when you insert a line break inside
a line comment, it will make the new line into an aligned line comment.

Working on tests, but want people to patch it in and try it - it's hard to
see if "feel" is right purely by looking at a test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60605

Files:
  clangd/CMakeLists.txt
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.cpp
  clangd/Format.h

Index: clangd/Format.h
===
--- /dev/null
+++ clangd/Format.h
@@ -0,0 +1,45 @@
+//===--- Format.h - automatic code formatting ---*- C++-*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Clangd uses clang-format for formatting operations.
+// This file adapts it to support new scenarios like format-on-type.
+//
+//===--===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FORMAT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FORMAT_H
+
+#include "Protocol.h"
+#include "clang/Format/Format.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace clangd {
+
+/// Applies limited formatting around new \p InsertedText.
+/// The \p Code already contains the updated text near \p Cursor, and may have
+/// had additional / characters (such as indentation) inserted by the editor.
+///
+/// Example breaking a line (^ is the cursor):
+/// === before newline is typed ===
+/// if(1){^}
+/// === after newline is typed and editor indents ===
+/// if(1){
+///   ^}
+/// === after formatIncremental(InsertedText="\n") ===
+/// if (1) {
+///   ^
+/// }
+tooling::Replacements formatIncremental(llvm::StringRef Code, unsigned Cursor,
+llvm::StringRef InsertedText,
+format::FormatStyle Style);
+
+} // namespace clangd
+} // namespace clang
+
+#endif
+
Index: clangd/Format.cpp
===
--- /dev/null
+++ clangd/Format.cpp
@@ -0,0 +1,228 @@
+//===--- Format.cpp -*- C++-*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "Format.h"
+#include "Logger.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/Support/Unicode.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+/// Append closing brackets )]} to \p Code to make it well-formed.
+/// Clang-format conservatively refuses to format files with unmatched brackets
+/// as it isn't sure where the errors are and so can't correct.
+/// When editing, it's reasonable to assume code before the cursor is complete.
+void closeBrackets(std::string , const format::FormatStyle ) {
+  SourceManagerForFile FileSM("dummy.cpp", Code);
+  auto  = FileSM.get();
+  FileID FID = SM.getMainFileID();
+  Lexer Lex(FID, SM.getBuffer(FID), SM, format::getFormattingLangOpts(Style));
+  Token Tok;
+  std::vector Brackets;
+  while (!Lex.LexFromRawLexer(Tok)) {
+switch(Tok.getKind()) {
+  case tok::l_paren:
+Brackets.push_back(')');
+break;
+  case tok::l_brace:
+Brackets.push_back('}');
+break;
+  case tok::l_square:
+Brackets.push_back(']');
+break;
+  case tok::r_paren:
+if (!Brackets.empty() && Brackets.back() == ')')
+  Brackets.pop_back();
+break;
+  case tok::r_brace:
+if (!Brackets.empty() && Brackets.back() == '}')
+  Brackets.pop_back();
+break;
+  case tok::r_square:
+if (!Brackets.empty() &&