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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D57739#1392453 , @sammccall wrote:

> 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 tests having narrow scope is a good thing - if we want system tests 
> > > that check clangdserver's behavior, they should test clangdserver.
> > >  Clients that don't go through clangdserver probably want formatting, but 
> > > an immediate cleanupAndFormat on each generated change isn't necessarily 
> > > the right way to do that.
> >
> >
> > My argument is that it's better to provide formatting by default in the 
> > public interface for **applying a single tweak**.
> >  I might be missing some use-cases, e.g. one that comes to mind is applying 
> > multiple tweaks in a row, in which case we'd want to format once and not 
> > for every tweak.
>
>
> If providing formatting was free, I'd be fine with this, but it leaks into 
> the public interface in two places: a) it requires the caller to plumb 
> through a formatting style, and b) it is another source of errors.
>
> For this particular interface it's more important that it's conceptually 
> clear and easy to implement than it is that it's easy to call - this is an 
> extension point.
>
> > My concerns are code duplication and ease of use for the clients. Having 
> > both `apply` and `applyAndFormat` (the latter being a non-virtual or a 
> > free-standing utility function) in the public interface of the `Tweak` 
> > would totally do it for me.
>
> I'd be happy with `applyAndFormat` as a free function - my main concern is 
> that formatting not be part of the scope of the class.


Given that clangdServer is the only client of `Tweaks` (if we don't do format 
in tests), I think it is fine to move out `applyAndFormat` functionality from 
`Tweaks`. I somehow agree that `applyAndFormat` is a standalone function.

>> However, I also think tests should format by default, not sure we agree on 
>> this.
>>  WDYT?
> 
> I'd rather they didn't format, but I don't think it will matter much and I'm 
> happy either way.
> 
> (Where it does matter, I'd rather have the differences between input/output 
> be a result of the tweak replacements, not of different formatting triggered 
> by a different token sequence.
>  I don't think there's much point in testing clang-format here, though we 
> should have one test that verifies we're calling it at all.)

Personally, I prefer not doing format in tests -- it would make writing 
testcases easier, it is annoying to spend time on figuring out **format-only** 
differences between actual code and expected code.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Sam McCall via Phabricator via cfe-commits]

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 tests having narrow scope is a good thing - if we want system tests 
> > that check clangdserver's behavior, they should test clangdserver.
> >  Clients that don't go through clangdserver probably want formatting, but 
> > an immediate cleanupAndFormat on each generated change isn't necessarily 
> > the right way to do that.
>
>
> My argument is that it's better to provide formatting by default in the 
> public interface for **applying a single tweak**.
>  I might be missing some use-cases, e.g. one that comes to mind is applying 
> multiple tweaks in a row, in which case we'd want to format once and not for 
> every tweak.


If providing formatting was free, I'd be fine with this, but it leaks into the 
public interface in two places: a) it requires the caller to plumb through a 
formatting style, and b) it is another source of errors.

For this particular interface it's more important that it's conceptually clear 
and easy to implement than it is that it's easy to call - this is an extension 
point.

> My concerns are code duplication and ease of use for the clients. Having both 
> `apply` and `applyAndFormat` (the latter being a non-virtual or a 
> free-standing utility function) in the public interface of the `Tweak` would 
> totally do it for me.

I'd be happy with `applyAndFormat` as a free function - my main concern is that 
formatting not be part of the scope of the class.

> However, I also think tests should format by default, not sure we agree on 
> this.
>  WDYT?

I'd rather they didn't format, but I don't think it will matter much and I'm 
happy either way.

(Where it does matter, I'd rather have the differences between input/output be 
a result of the tweak replacements, not of different formatting triggered by a 
different token sequence.
I don't think there's much point in testing clang-format here, though we should 
have one test that verifies we're calling it at all.)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 clangdserver's behavior, they should test clangdserver.
>  Clients that don't go through clangdserver probably want formatting, but an 
> immediate cleanupAndFormat on each generated change isn't necessarily the 
> right way to do that.


My argument is that it's better to provide formatting by default in the public 
interface for **applying a single tweak**.
I might be missing some use-cases, e.g. one that comes to mind is applying 
multiple tweaks in a row, in which case we'd want to format once and not for 
every tweak.

My concerns are code duplication and ease of use for the clients. Having both 
`apply` and `applyAndFormat` (the latter being a non-virtual or a free-standing 
utility function) in the public interface of the `Tweak` would totally do it 
for me.
However, I also think tests should format by default, not sure we agree on this.
WDYT?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Sam McCall via Phabricator via cfe-commits]

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 styling or failing on format errors. Can we consider moving the 
> > formatting to clangdserver as originally planned?
>
>
> `clang-format` seems to be somewhat stable, do we actually expect that to be 
> a large problem in practice?
>  On the other side, I can't imagine any clients that don't need formatting? 
> E.g. it's nice when tests give the same results as one would see in the final 
> clangd and tests don't go through `ClangdServer`.


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 clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an 
immediate cleanupAndFormat on each generated change isn't necessarily the right 
way to do that.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 as originally planned?


`clang-format` seems to be somewhat stable, do we actually expect that to be a 
large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. 
it's nice when tests give the same results as one would see in the final clangd 
and tests don't go through `ClangdServer`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Sam McCall via Phabricator via cfe-commits]

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 formatting? I.e.
> >
> >   class Tweak {
> >  Replacements apply() {  // This is now non-virtual.
> >auto Replacements = doApply();
> >return cleanupAndFormat(Replacements);
> >  }
> >  
> >   protected:
> >  virtual Replacements doApply() = 0; // inheritors should implement 
> > this now. Note: the name is terrible, suggestions are welcome.
> >   };
> >
> >
> > This would ensure the callers don't need to deal with the formatting on 
> > their own.
>
>
> 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.


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 as originally planned?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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



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

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

[Haojian Wu via Phabricator via cfe-commits]

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
  clangd/Compiler.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/refactor/Tweak.cpp
  clangd/refactor/Tweak.h
  clangd/refactor/tweaks/SwapIfBranches.cpp
  unittests/clangd/TweakTests.cpp

Index: unittests/clangd/TweakTests.cpp
===
--- unittests/clangd/TweakTests.cpp
+++ unittests/clangd/TweakTests.cpp
@@ -98,7 +98,7 @@
   auto T = prepareTweak(ID, S);
   if (!T)
 return T.takeError();
-  auto Replacements = (*T)->apply(S);
+  auto Replacements = (*T)->apply(S, clang::format::getLLVMStyle());
   if (!Replacements)
 return Replacements.takeError();
   return applyAllReplacements(Code.code(), *Replacements);
@@ -127,12 +127,40 @@
 
   llvm::StringLiteral Input = R"cpp(
 void test() {
-  ^if (true) { return 100; } else { continue; }
+  ^if (true) {
+return 100;
+  } else {
+continue;
+  }
 }
   )cpp";
   llvm::StringLiteral Output = R"cpp(
 void test() {
-  if (true) { continue; } else { return 100; }
+  if (true) {
+continue;
+  } else {
+return 100;
+  }
+}
+  )cpp";
+  checkTransform(ID, Input, Output);
+
+  Input = R"cpp(
+void test() {
+  ^if () {
+return 100;
+  } else {
+continue;
+  }
+}
+  )cpp";
+  Output = R"cpp(
+void test() {
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
@@ -144,7 +172,11 @@
   )cpp";
   Output = R"cpp(
 void test() {
-  if () { continue; } else { return 100; }
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -37,9 +37,11 @@
   const char *id() const override final;
 
   bool prepare(const Selection ) override;
-  Expected apply(const Selection ) override;
   std::string title() const override;
 
+protected:
+  Expected execute(const Selection ) override;
+
 private:
   const IfStmt *If = nullptr;
 };
@@ -60,7 +62,8 @@
  dyn_cast_or_null(If->getElse());
 }
 
-Expected SwapIfBranches::apply(const Selection ) {
+Expected
+SwapIfBranches::execute(const Selection ) {
   auto  = Inputs.AST.getASTContext();
   auto  = Ctx.getSourceManager();
 
Index: clangd/refactor/Tweak.h
===
--- clangd/refactor/Tweak.h
+++ clangd/refactor/Tweak.h
@@ -22,6 +22,7 @@
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "Selection.h"
+#include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
@@ -47,7 +48,7 @@
 ParsedAST 
 /// A location of the cursor in the editor.
 SourceLocation Cursor;
-// The AST nodes that were selected.
+/// The AST nodes that were selected.
 SelectionTree ASTSelection;
 // FIXME: provide a way to get sources and ASTs for other files.
   };
@@ -63,13 +64,20 @@
   /// should be moved into 'apply'.
   /// Returns true iff the action is available and apply() can be called on it.
   virtual bool prepare(const Selection ) = 0;
-  /// Run the second stage of the action that would produce the actual changes.
+  /// Format and apply the actual changes generated from the second stage of the
+  /// action.
   /// EXPECTS: prepare() was called and returned true.
-  virtual Expected apply(const Selection ) = 0;
+  Expected apply(const Selection ,
+const format::FormatStyle );
   /// A one-line title of the action that should be shown to the users in the
   /// UI.
   /// EXPECTS: prepare() was called and returned true.
   virtual std::string title() const = 0;
+
+protected:
+  /// Run the second stage of the action that would produce the actual changes.
+  /// EXPECTS: prepare() was called and returned true.
+  virtual Expected execute(const Selection ) = 0;
 };
 
 // All tweaks must be registered in the .cpp file next to their definition.
Index: clangd/refactor/Tweak.cpp
===
--- clangd/refactor/Tweak.cpp
+++ clangd/refactor/Tweak.cpp
@@ -46,6 +46,14 @@
   Cursor = SM.getComposedLoc(SM.getMainFileID(), RangeBegin);
 }
 
+Expected Tweak::apply(const Selection ,
+ const format::FormatStyle ) {
+  auto RawReplacements = execute(Sel);
+  if (!RawReplacements)
+return RawReplacements;
+  

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

[Haojian Wu via Phabricator via cfe-commits]

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:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/Compiler.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/refactor/Tweak.cpp
  clangd/refactor/Tweak.h
  clangd/refactor/tweaks/SwapIfBranches.cpp
  unittests/clangd/TweakTests.cpp

Index: unittests/clangd/TweakTests.cpp
===
--- unittests/clangd/TweakTests.cpp
+++ unittests/clangd/TweakTests.cpp
@@ -53,7 +53,8 @@
   auto CheckOver = [&](Range Selection) {
 unsigned Begin = cantFail(positionToOffset(Code.code(), Selection.start));
 unsigned End = cantFail(positionToOffset(Code.code(), Selection.end));
-auto T = prepareTweak(ID, Tweak::Selection(AST, Begin, End));
+auto T = prepareTweak(
+ID, Tweak::Selection(AST, Begin, End));
 if (Available)
   EXPECT_THAT_EXPECTED(T, Succeeded())
   << "code is " << markRange(Code.code(), Selection);
@@ -98,7 +99,7 @@
   auto T = prepareTweak(ID, S);
   if (!T)
 return T.takeError();
-  auto Replacements = (*T)->apply(S);
+  auto Replacements = (*T)->apply(S, clang::format::getLLVMStyle());
   if (!Replacements)
 return Replacements.takeError();
   return applyAllReplacements(Code.code(), *Replacements);
@@ -127,12 +128,40 @@
 
   llvm::StringLiteral Input = R"cpp(
 void test() {
-  ^if (true) { return 100; } else { continue; }
+  ^if (true) {
+return 100;
+  } else {
+continue;
+  }
 }
   )cpp";
   llvm::StringLiteral Output = R"cpp(
 void test() {
-  if (true) { continue; } else { return 100; }
+  if (true) {
+continue;
+  } else {
+return 100;
+  }
+}
+  )cpp";
+  checkTransform(ID, Input, Output);
+
+  Input = R"cpp(
+void test() {
+  ^if () {
+return 100;
+  } else {
+continue;
+  }
+}
+  )cpp";
+  Output = R"cpp(
+void test() {
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
@@ -144,7 +173,11 @@
   )cpp";
   Output = R"cpp(
 void test() {
-  if () { continue; } else { return 100; }
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -37,9 +37,11 @@
   const char *id() const override final;
 
   bool prepare(const Selection ) override;
-  Expected apply(const Selection ) override;
   std::string title() const override;
 
+protected:
+  Expected execute(const Selection ) override;
+
 private:
   const IfStmt *If = nullptr;
 };
@@ -60,7 +62,8 @@
  dyn_cast_or_null(If->getElse());
 }
 
-Expected SwapIfBranches::apply(const Selection ) {
+Expected
+SwapIfBranches::execute(const Selection ) {
   auto  = Inputs.AST.getASTContext();
   auto  = Ctx.getSourceManager();
 
Index: clangd/refactor/Tweak.h
===
--- clangd/refactor/Tweak.h
+++ clangd/refactor/Tweak.h
@@ -22,6 +22,7 @@
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "Selection.h"
+#include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
@@ -47,7 +48,7 @@
 ParsedAST 
 /// A location of the cursor in the editor.
 SourceLocation Cursor;
-// The AST nodes that were selected.
+/// The AST nodes that were selected.
 SelectionTree ASTSelection;
 // FIXME: provide a way to get sources and ASTs for other files.
   };
@@ -63,13 +64,20 @@
   /// should be moved into 'apply'.
   /// Returns true iff the action is available and apply() can be called on it.
   virtual bool prepare(const Selection ) = 0;
-  /// Run the second stage of the action that would produce the actual changes.
+  /// Format and apply the actual changes generated from the second stage of the
+  /// action.
   /// EXPECTS: prepare() was called and returned true.
-  virtual Expected apply(const Selection ) = 0;
+  Expected apply(const Selection ,
+const format::FormatStyle );
   /// A one-line title of the action that should be shown to the users in the
   /// UI.
   /// EXPECTS: prepare() was called and returned true.
   virtual std::string title() const = 0;
+
+protected:
+  /// Run the second stage of the action that would produce the actual changes.
+  /// EXPECTS: prepare() was called and returned true.
+  virtual Expected execute(const Selection ) = 0;
 };
 
 // All tweaks must 

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

[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! Also a few NITs.




Comment at: clangd/ClangdServer.cpp:155
 Opts.ClangTidyOpts = ClangTidyOptProvider->getOptions(File);
+  // FIXME: cache this
+  Opts.Style =

NIT: put `.` to the end of the comment.



Comment at: clangd/ClangdServer.cpp:378
   return CB(A.takeError());
 // FIXME: run formatter on top of resulting replacements.
+return CB((*A)->apply(*Selection, InpAST->Inputs.Opts.Style));

This FIXME is stale now, remove?



Comment at: clangd/Format.h:19
+inline llvm::Expected
+cleanupAndFormat(StringRef Code, const tooling::Replacements ,
+ const format::FormatStyle ) {

NIT: Maybe move to `SourceCode.h`? We have helpers to turn replacements into 
edits there, having this would be a good addition.
Or inline now that this function has only one call-site?

The extra file does not seem to carry its weight.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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 `execute()` should not do formatting on its 
> own.
SG, this also makes the `format` intention of `apply` more explicitly.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/Compiler.h
  clangd/refactor/Tweak.cpp
  clangd/refactor/Tweak.h
  clangd/refactor/tweaks/SwapIfBranches.cpp
  unittests/clangd/TweakTests.cpp

Index: unittests/clangd/TweakTests.cpp
===
--- unittests/clangd/TweakTests.cpp
+++ unittests/clangd/TweakTests.cpp
@@ -53,7 +53,8 @@
   auto CheckOver = [&](Range Selection) {
 unsigned Begin = cantFail(positionToOffset(Code.code(), Selection.start));
 unsigned End = cantFail(positionToOffset(Code.code(), Selection.end));
-auto T = prepareTweak(ID, Tweak::Selection(AST, Begin, End));
+auto T = prepareTweak(
+ID, Tweak::Selection(AST, Begin, End));
 if (Available)
   EXPECT_THAT_EXPECTED(T, Succeeded())
   << "code is " << markRange(Code.code(), Selection);
@@ -98,7 +99,7 @@
   auto T = prepareTweak(ID, S);
   if (!T)
 return T.takeError();
-  auto Replacements = (*T)->apply(S);
+  auto Replacements = (*T)->apply(S, clang::format::getLLVMStyle());
   if (!Replacements)
 return Replacements.takeError();
   return applyAllReplacements(Code.code(), *Replacements);
@@ -127,12 +128,40 @@
 
   llvm::StringLiteral Input = R"cpp(
 void test() {
-  ^if (true) { return 100; } else { continue; }
+  ^if (true) {
+return 100;
+  } else {
+continue;
+  }
 }
   )cpp";
   llvm::StringLiteral Output = R"cpp(
 void test() {
-  if (true) { continue; } else { return 100; }
+  if (true) {
+continue;
+  } else {
+return 100;
+  }
+}
+  )cpp";
+  checkTransform(ID, Input, Output);
+
+  Input = R"cpp(
+void test() {
+  ^if () {
+return 100;
+  } else {
+continue;
+  }
+}
+  )cpp";
+  Output = R"cpp(
+void test() {
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
@@ -144,7 +173,11 @@
   )cpp";
   Output = R"cpp(
 void test() {
-  if () { continue; } else { return 100; }
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -37,9 +37,11 @@
   const char *id() const override final;
 
   bool prepare(const Selection ) override;
-  Expected apply(const Selection ) override;
   std::string title() const override;
 
+protected:
+  Expected execute(const Selection ) override;
+
 private:
   const IfStmt *If = nullptr;
 };
@@ -60,7 +62,8 @@
  dyn_cast_or_null(If->getElse());
 }
 
-Expected SwapIfBranches::apply(const Selection ) {
+Expected
+SwapIfBranches::execute(const Selection ) {
   auto  = Inputs.AST.getASTContext();
   auto  = Ctx.getSourceManager();
 
Index: clangd/refactor/Tweak.h
===
--- clangd/refactor/Tweak.h
+++ clangd/refactor/Tweak.h
@@ -22,6 +22,7 @@
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "Selection.h"
+#include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
@@ -47,7 +48,7 @@
 ParsedAST 
 /// A location of the cursor in the editor.
 SourceLocation Cursor;
-// The AST nodes that were selected.
+/// The AST nodes that were selected.
 SelectionTree ASTSelection;
 // FIXME: provide a way to get sources and ASTs for other files.
   };
@@ -63,13 +64,20 @@
   /// should be moved into 'apply'.
   /// Returns true iff the action is available and apply() can be called on it.
   virtual bool prepare(const Selection ) = 0;
-  /// Run the second stage of the action that would produce the actual changes.
+  /// Format and apply the actual changes generated from the second stage of the
+  /// action.
   /// EXPECTS: prepare() was called and returned true.
-  virtual Expected apply(const Selection ) = 0;
+  Expected apply(const Selection ,
+const format::FormatStyle );
   /// A one-line title of the action that should be shown to the users in the
   /// UI.
   /// EXPECTS: prepare() was called and returned true.
   virtual std::string title() const = 0;
+
+protected:
+  /// Run the second stage of the action that would produce the actual changes.
+  /// EXPECTS: prepare() was called and returned true.
+  virtual Expected execute(const Selection ) = 0;
 };
 
 // All tweaks must be registered in the .cpp file next to 

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

[Haojian Wu via Phabricator via cfe-commits]

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
  clangd/Compiler.h
  clangd/Format.h
  clangd/refactor/Tweak.cpp
  clangd/refactor/Tweak.h
  clangd/refactor/tweaks/SwapIfBranches.cpp
  unittests/clangd/TweakTests.cpp

Index: unittests/clangd/TweakTests.cpp
===
--- unittests/clangd/TweakTests.cpp
+++ unittests/clangd/TweakTests.cpp
@@ -53,7 +53,8 @@
   auto CheckOver = [&](Range Selection) {
 unsigned Begin = cantFail(positionToOffset(Code.code(), Selection.start));
 unsigned End = cantFail(positionToOffset(Code.code(), Selection.end));
-auto T = prepareTweak(ID, Tweak::Selection(AST, Begin, End));
+auto T = prepareTweak(
+ID, Tweak::Selection(AST, Begin, End));
 if (Available)
   EXPECT_THAT_EXPECTED(T, Succeeded())
   << "code is " << markRange(Code.code(), Selection);
@@ -98,7 +99,7 @@
   auto T = prepareTweak(ID, S);
   if (!T)
 return T.takeError();
-  auto Replacements = (*T)->apply(S);
+  auto Replacements = (*T)->apply(S, clang::format::getLLVMStyle());
   if (!Replacements)
 return Replacements.takeError();
   return applyAllReplacements(Code.code(), *Replacements);
@@ -127,12 +128,40 @@
 
   llvm::StringLiteral Input = R"cpp(
 void test() {
-  ^if (true) { return 100; } else { continue; }
+  ^if (true) {
+return 100;
+  } else {
+continue;
+  }
 }
   )cpp";
   llvm::StringLiteral Output = R"cpp(
 void test() {
-  if (true) { continue; } else { return 100; }
+  if (true) {
+continue;
+  } else {
+return 100;
+  }
+}
+  )cpp";
+  checkTransform(ID, Input, Output);
+
+  Input = R"cpp(
+void test() {
+  ^if () {
+return 100;
+  } else {
+continue;
+  }
+}
+  )cpp";
+  Output = R"cpp(
+void test() {
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
@@ -144,7 +173,11 @@
   )cpp";
   Output = R"cpp(
 void test() {
-  if () { continue; } else { return 100; }
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -37,9 +37,11 @@
   const char *id() const override final;
 
   bool prepare(const Selection ) override;
-  Expected apply(const Selection ) override;
   std::string title() const override;
 
+protected:
+  Expected execute(const Selection ) override;
+
 private:
   const IfStmt *If = nullptr;
 };
@@ -60,7 +62,8 @@
  dyn_cast_or_null(If->getElse());
 }
 
-Expected SwapIfBranches::apply(const Selection ) {
+Expected
+SwapIfBranches::execute(const Selection ) {
   auto  = Inputs.AST.getASTContext();
   auto  = Ctx.getSourceManager();
 
Index: clangd/refactor/Tweak.h
===
--- clangd/refactor/Tweak.h
+++ clangd/refactor/Tweak.h
@@ -22,6 +22,7 @@
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "Selection.h"
+#include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
@@ -47,7 +48,7 @@
 ParsedAST 
 /// A location of the cursor in the editor.
 SourceLocation Cursor;
-// The AST nodes that were selected.
+/// The AST nodes that were selected.
 SelectionTree ASTSelection;
 // FIXME: provide a way to get sources and ASTs for other files.
   };
@@ -63,13 +64,20 @@
   /// should be moved into 'apply'.
   /// Returns true iff the action is available and apply() can be called on it.
   virtual bool prepare(const Selection ) = 0;
-  /// Run the second stage of the action that would produce the actual changes.
+  /// Format and apply the actual changes generated from the second stage of the
+  /// action.
   /// EXPECTS: prepare() was called and returned true.
-  virtual Expected apply(const Selection ) = 0;
+  Expected apply(const Selection ,
+const format::FormatStyle );
   /// A one-line title of the action that should be shown to the users in the
   /// UI.
   /// EXPECTS: prepare() was called and returned true.
   virtual std::string title() const = 0;
+
+protected:
+  /// Run the second stage of the action that would produce the actual changes.
+  /// EXPECTS: prepare() was called and returned true.
+  virtual Expected execute(const Selection ) = 0;
 };
 
 // All tweaks must be registered in the .cpp file next to their definition.
Index: 

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

[Ilya Biryukov via Phabricator via cfe-commits]

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`, I also have a few 
> candidates in mind `perform`, `invoke`.
`execute()` LGTM!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Ilya Biryukov via Phabricator via cfe-commits]

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()` should not do formatting on its own.



Comment at: clangd/refactor/Tweak.h:72
+  /// action.
+  Expected apply(const Selection );
   /// A one-line title of the action that should be shown to the users in the

Could you duplicate the `EXPECTS:` comments here? It's an important part of the 
public API.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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 better than `doApply`, I also have a few 
candidates in mind `perform`, `invoke`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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
  clangd/Compiler.h
  clangd/refactor/Tweak.cpp
  clangd/refactor/Tweak.h
  clangd/refactor/tweaks/SwapIfBranches.cpp
  unittests/clangd/TweakTests.cpp

Index: unittests/clangd/TweakTests.cpp
===
--- unittests/clangd/TweakTests.cpp
+++ unittests/clangd/TweakTests.cpp
@@ -53,7 +53,8 @@
   auto CheckOver = [&](Range Selection) {
 unsigned Begin = cantFail(positionToOffset(Code.code(), Selection.start));
 unsigned End = cantFail(positionToOffset(Code.code(), Selection.end));
-auto T = prepareTweak(ID, Tweak::Selection(AST, Begin, End));
+auto T = prepareTweak(
+ID, Tweak::Selection(AST, Begin, End, clang::format::getLLVMStyle()));
 if (Available)
   EXPECT_THAT_EXPECTED(T, Succeeded())
   << "code is " << markRange(Code.code(), Selection);
@@ -93,7 +94,7 @@
   ParsedAST AST = TU.build();
   unsigned Begin = cantFail(positionToOffset(Code.code(), SelectionRng.start));
   unsigned End = cantFail(positionToOffset(Code.code(), SelectionRng.end));
-  Tweak::Selection S(AST, Begin, End);
+  Tweak::Selection S(AST, Begin, End, clang::format::getLLVMStyle());
 
   auto T = prepareTweak(ID, S);
   if (!T)
@@ -127,12 +128,40 @@
 
   llvm::StringLiteral Input = R"cpp(
 void test() {
-  ^if (true) { return 100; } else { continue; }
+  ^if (true) {
+return 100;
+  } else {
+continue;
+  }
 }
   )cpp";
   llvm::StringLiteral Output = R"cpp(
 void test() {
-  if (true) { continue; } else { return 100; }
+  if (true) {
+continue;
+  } else {
+return 100;
+  }
+}
+  )cpp";
+  checkTransform(ID, Input, Output);
+
+  Input = R"cpp(
+void test() {
+  ^if () {
+return 100;
+  } else {
+continue;
+  }
+}
+  )cpp";
+  Output = R"cpp(
+void test() {
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
@@ -144,7 +173,11 @@
   )cpp";
   Output = R"cpp(
 void test() {
-  if () { continue; } else { return 100; }
+  if () {
+continue;
+  } else {
+return 100;
+  }
 }
   )cpp";
   checkTransform(ID, Input, Output);
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -37,9 +37,11 @@
   const char *id() const override final;
 
   bool prepare(const Selection ) override;
-  Expected apply(const Selection ) override;
   std::string title() const override;
 
+protected:
+  Expected execute(const Selection ) override;
+
 private:
   const IfStmt *If = nullptr;
 };
@@ -60,7 +62,8 @@
  dyn_cast_or_null(If->getElse());
 }
 
-Expected SwapIfBranches::apply(const Selection ) {
+Expected
+SwapIfBranches::execute(const Selection ) {
   auto  = Inputs.AST.getASTContext();
   auto  = Ctx.getSourceManager();
 
Index: clangd/refactor/Tweak.h
===
--- clangd/refactor/Tweak.h
+++ clangd/refactor/Tweak.h
@@ -40,16 +40,20 @@
 public:
   /// Input to prepare and apply tweaks.
   struct Selection {
-Selection(ParsedAST , unsigned RangeBegin, unsigned RangeEnd);
+Selection(ParsedAST , unsigned RangeBegin, unsigned RangeEnd,
+  format::FormatStyle Style);
 /// The text of the active document.
 llvm::StringRef Code;
 /// Parsed AST of the active file.
 ParsedAST 
 /// A location of the cursor in the editor.
 SourceLocation Cursor;
-// The AST nodes that were selected.
+/// The AST nodes that were selected.
 SelectionTree ASTSelection;
 // FIXME: provide a way to get sources and ASTs for other files.
+
+/// The style to format generated changes.
+format::FormatStyle Style;
   };
   virtual ~Tweak() = default;
   /// A unique id of the action, it is always equal to the name of the class
@@ -63,13 +67,18 @@
   /// should be moved into 'apply'.
   /// Returns true iff the action is available and apply() can be called on it.
   virtual bool prepare(const Selection ) = 0;
-  /// Run the second stage of the action that would produce the actual changes.
-  /// EXPECTS: prepare() was called and returned true.
-  virtual Expected apply(const Selection ) = 0;
+  /// Format and apply the actual changes generated from the second stage of the
+  /// action.
+  Expected apply(const Selection );
   /// A one-line title of the action that should be shown to the users in the
   /// UI.
   /// EXPECTS: prepare() was called and returned true.
   virtual 

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

[Ilya Biryukov via Phabricator via cfe-commits]

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, since we have just a single tweak now and changing the 
interface costs nothing.
The important part is making it **hard** to misuse the interface and forcing 
formatting on all the users of the interface is a significant burden. E.g. 
other users of `ClangdServer` (tests, users of clangd C++ API)  shouldn't worry 
about formatting on their own.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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.
>auto Replacements = doApply();
>return cleanupAndFormat(Replacements);
>  }
>  
>   protected:
>  virtual Replacements doApply() = 0; // inheritors should implement this 
> now. Note: the name is terrible, suggestions are welcome.
>   };
>
>
> This would ensure the callers don't need to deal with the formatting on their 
> own.


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.




Comment at: clangd/ClangdServer.cpp:366
+  auto Style = getFormatStyle(Code, File);
+  if (!Style)
+return;

ilya-biryukov wrote:
> ioeric wrote:
> > hokein wrote:
> > > not sure the err-handling strategy here -- maybe if this is failed, we 
> > > still apply replacements (without formatting), rather than stopping.
> > You should use `getFormatStyleForFile` from SourceCode.h
> > not sure the err-handling strategy here -- maybe if this is failed, we 
> > still apply replacements (without formatting), rather than stopping.
> Returning an error seems fine, this probably shouldn't happen under normal 
> conditions and failing early means we're more likely to find the root-cause 
> of the problem.
> You should use getFormatStyleForFile from SourceCode.h

Thanks for pointing it out! Didn't notice this method. I think we need to cache 
it somehow rather than calling this method every time.

> Returning an error seems fine, this probably shouldn't happen under normal 
> conditions and failing early means we're more likely to find the root-cause 
> of the problem.

I switched to use getFormatStyleForFile, it seems a reasonable API. 



Comment at: clangd/ClangdServer.h:267
+  /// slow. Think of a way to cache this.
+  llvm::Expected getFormatStyle(llvm::StringRef Code,
+ PathRef File);

ilya-biryukov wrote:
> NIT: could we swap the parameters for consistency with the other functions in 
> this class (i.e. `File` goes first)
removed, not needed any more.



Comment at: unittests/clangd/TweakTests.cpp:82
+llvm::Expected apply(StringRef ID, llvm::StringRef Input,
+  bool Format) {
   Annotations Code(Input);

ilya-biryukov wrote:
> NIT: remove the parameter, this should be the default.
> At least until we run into the checks that deliberately don't want the 
> formatting
I'm ok to make this change. I tempt to keep it because it would make writing 
test code easier -- if we always format the code, we might spend time on 
figuring out the format diff between `Before` and `After`, which is not worth.



Comment at: unittests/clangd/TweakTests.cpp:110
+Code.code(), *Replacements,
+clang::format::getGoogleStyle(::clang::format::FormatStyle::LK_Cpp));
+if (!Replacements)

ilya-biryukov wrote:
> NIT: maybe prefer LLVM style? To avoid context-switching.
good catch.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Format.h
  unittests/clangd/TweakTests.cpp

Index: unittests/clangd/TweakTests.cpp
===
--- unittests/clangd/TweakTests.cpp
+++ unittests/clangd/TweakTests.cpp
@@ -9,6 +9,7 @@
 
 #include "Annotations.h"
 #include "SourceCode.h"
+#include "Format.h"
 #include "TestTU.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/Expr.h"
@@ -77,7 +78,8 @@
 void checkNotAvailable(StringRef ID, llvm::StringRef Input) {
   return checkAvailable(ID, Input, /*Available=*/false);
 }
-llvm::Expected apply(StringRef ID, llvm::StringRef Input) {
+llvm::Expected apply(StringRef ID, llvm::StringRef Input,
+  bool Format) {
   Annotations Code(Input);
   Range SelectionRng;
   if (Code.points().size() != 0) {
@@ -102,12 +104,19 @@
   auto Replacements = (*T)->apply(S);
   if (!Replacements)
 return Replacements.takeError();
+  if (Format) {
+Replacements = cleanupAndFormat(
+Code.code(), *Replacements,
+clang::format::getLLVMStyle());
+if (!Replacements)
+  return Replacements.takeError();
+  }
   return applyAllReplacements(Code.code(), *Replacements);
 }
 
 void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
-llvm::StringRef Output) {
-  EXPECT_THAT_EXPECTED(apply(ID, Input), HasValue(Output))
+llvm::StringRef Output, bool Format = false) {
+  EXPECT_THAT_EXPECTED(apply(ID, Input, Format), HasValue(Output))
   << "action id is" << ID;
 }
 
@@ -150,6 +159,22 @@
   )cpp";
   checkTransform(ID, Input, Output);
 
+  Input = R"cpp(
+void test() {
+  ^if () { return 100; } else { continue; }
+}
+  )cpp";
+  Output = R"cpp(
+void test() {
+  if () {
+continue;
+  } else {
+return 100;
+  }
+}
+  )cpp";
+  checkTransform(ID, Input, Output, /*Format=*/true);
+
   // Available in subexpressions of the condition.
   checkAvailable(ID, R"cpp(
 void test() {
Index: clangd/Format.h
===
--- /dev/null
+++ clangd/Format.h
@@ -0,0 +1,28 @@
+//===--- Format.h *- 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 "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+// Cleanup and format the given replacements.
+inline llvm::Expected
+cleanupAndFormat(StringRef Code, const tooling::Replacements ,
+ const format::FormatStyle ) {
+  auto CleanReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  if (!CleanReplaces)
+return CleanReplaces;
+  return formatReplacements(Code, std::move(*CleanReplaces), Style);
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -23,6 +23,7 @@
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "refactor/Tweak.h"
+#include "clang/Format/Format.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/FunctionExtras.h"
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -10,6 +10,7 @@
 #include "ClangdUnit.h"
 #include "CodeComplete.h"
 #include "FindSymbols.h"
+#include "Format.h"
 #include "Headers.h"
 #include "SourceCode.h"
 #include "Trace.h"
@@ -362,8 +363,9 @@
 
 void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
-  Expected InpAST) {
+  auto Action = [Sel, this](decltype(CB) CB, std::string File,
+std::string TweakID,
+Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 auto Selection = tweakSelection(Sel, *InpAST);
@@ -372,8 +374,15 @@
 auto A = prepareTweak(TweakID, *Selection);
 if (!A)
   return CB(A.takeError());
-// FIXME: run formatter on top of resulting replacements.
-return CB((*A)->apply(*Selection));
+auto 

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 
> > still apply replacements (without formatting), rather than stopping.
> You should use `getFormatStyleForFile` from SourceCode.h
> not sure the err-handling strategy here -- maybe if this is failed, we still 
> apply replacements (without formatting), rather than stopping.
Returning an error seems fine, this probably shouldn't happen under normal 
conditions and failing early means we're more likely to find the root-cause of 
the problem.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Ilya Biryukov via Phabricator via cfe-commits]

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);
 }
  
  protected:
 virtual Replacements doApply() = 0; // inheritors should implement this 
now. Note: the name is terrible, suggestions are welcome.
  };

This would ensure the callers don't need to deal with the formatting on their 
own.




Comment at: clangd/ClangdServer.cpp:362
 
-void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
+void ClangdServer::applyTweak(StringRef Code, PathRef File, Range Sel,
+  StringRef TweakID,

We can get the contents from `InpAST.Inputs.Contents`, no need to add an extra 
parameter.
Moving `getFormatStyle()` into the callback body should be ok.



Comment at: clangd/ClangdServer.cpp:500
+llvm::Expected
+ClangdServer::getFormatStyle(llvm::StringRef Code, PathRef File) {
+  return format::getStyle(format::DefaultFormatStyle, File,

Could this function get a `vfs::FileSystem` as a parameter?
Would server as a hint to the readers that it accesses the filesystem and allow 
to reuse vfs instances when they're already available (the apply tweaks case)



Comment at: clangd/ClangdServer.h:267
+  /// slow. Think of a way to cache this.
+  llvm::Expected getFormatStyle(llvm::StringRef Code,
+ PathRef File);

NIT: could we swap the parameters for consistency with the other functions in 
this class (i.e. `File` goes first)



Comment at: clangd/Format.h:29
+} // namespace clang
\ No newline at end of file


NIT: add newline at the end of file.



Comment at: unittests/clangd/TweakTests.cpp:82
+llvm::Expected apply(StringRef ID, llvm::StringRef Input,
+  bool Format) {
   Annotations Code(Input);

NIT: remove the parameter, this should be the default.
At least until we run into the checks that deliberately don't want the 
formatting



Comment at: unittests/clangd/TweakTests.cpp:110
+Code.code(), *Replacements,
+clang::format::getGoogleStyle(::clang::format::FormatStyle::LK_Cpp));
+if (!Replacements)

NIT: maybe prefer LLVM style? To avoid context-switching.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Eric Liu via Phabricator via cfe-commits]

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 (without formatting), rather than stopping.
You should use `getFormatStyleForFile` from SourceCode.h


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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 
apply replacements (without formatting), rather than stopping.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739



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

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

[Haojian Wu via Phabricator via cfe-commits]

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

Index: unittests/clangd/TweakTests.cpp
===
--- unittests/clangd/TweakTests.cpp
+++ unittests/clangd/TweakTests.cpp
@@ -9,6 +9,7 @@
 
 #include "Annotations.h"
 #include "SourceCode.h"
+#include "Format.h"
 #include "TestTU.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/Expr.h"
@@ -77,7 +78,8 @@
 void checkNotAvailable(StringRef ID, llvm::StringRef Input) {
   return checkAvailable(ID, Input, /*Available=*/false);
 }
-llvm::Expected apply(StringRef ID, llvm::StringRef Input) {
+llvm::Expected apply(StringRef ID, llvm::StringRef Input,
+  bool Format) {
   Annotations Code(Input);
   Range SelectionRng;
   if (Code.points().size() != 0) {
@@ -102,12 +104,19 @@
   auto Replacements = (*T)->apply(S);
   if (!Replacements)
 return Replacements.takeError();
+  if (Format) {
+Replacements = cleanupAndFormat(
+Code.code(), *Replacements,
+clang::format::getGoogleStyle(::clang::format::FormatStyle::LK_Cpp));
+if (!Replacements)
+  return Replacements.takeError();
+  }
   return applyAllReplacements(Code.code(), *Replacements);
 }
 
 void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
-llvm::StringRef Output) {
-  EXPECT_THAT_EXPECTED(apply(ID, Input), HasValue(Output))
+llvm::StringRef Output, bool Format = false) {
+  EXPECT_THAT_EXPECTED(apply(ID, Input, Format), HasValue(Output))
   << "action id is" << ID;
 }
 
@@ -150,6 +159,22 @@
   )cpp";
   checkTransform(ID, Input, Output);
 
+  Input = R"cpp(
+void test() {
+  ^if () { return 100; } else { continue; }
+}
+  )cpp";
+  Output = R"cpp(
+void test() {
+  if () {
+continue;
+  } else {
+return 100;
+  }
+}
+  )cpp";
+  checkTransform(ID, Input, Output, /*Format=*/true);
+
   // Available in subexpressions of the condition.
   checkAvailable(ID, R"cpp(
 void test() {
Index: clangd/Format.h
===
--- /dev/null
+++ clangd/Format.h
@@ -0,0 +1,28 @@
+//===--- Format.h --*- 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 "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+// Cleanup and format the given replacements.
+inline llvm::Expected
+cleanupAndFormat(StringRef Code, const tooling::Replacements ,
+ const format::FormatStyle ) {
+  auto CleanReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  if (!CleanReplaces)
+return CleanReplaces;
+  return formatReplacements(Code, std::move(*CleanReplaces), Style);
+}
+
+} // namespace clangd
+} // namespace clang
\ No newline at end of file
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -23,6 +23,7 @@
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "refactor/Tweak.h"
+#include "clang/Format/Format.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/FunctionExtras.h"
@@ -223,7 +224,7 @@
Callback> CB);
 
   /// Apply the code tweak with a specified \p ID.
-  void applyTweak(PathRef File, Range Sel, StringRef ID,
+  void applyTweak(StringRef Code, PathRef File, Range Sel, StringRef ID,
   Callback CB);
 
   /// Only for testing purposes.
@@ -257,12 +258,15 @@
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
 private:
-  /// FIXME: This stats several files to find a .clang-format file. I/O can be
-  /// slow. Think of a way to cache this.
   llvm::Expected
   formatCode(llvm::StringRef Code, PathRef File,
  ArrayRef Ranges);
 
+  /// FIXME: This stats several files to find a .clang-format file. I/O can be
+  /// slow. Think of a way to cache this.
+  llvm::Expected getFormatStyle(llvm::StringRef Code,
+ PathRef File);
+
   tooling::CompileCommand getCompileCommand(PathRef File);
 
   const GlobalCompilationDatabase 
Index: clangd/ClangdServer.cpp