Re: [PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Sam McCall via cfe-commits]

Thanks!
I suspect shared libraries makes a difference here, because the tweaks use
an unusual library type (object library) to force linking.

On Fri, Feb 1, 2019 at 3:01 AM Nathan Ridge via Phabricator <
revi...@reviews.llvm.org> wrote:

> nridge added a comment.
>
> Fix here: https://reviews.llvm.org/D57560
>
>
> Repository:
>   rL LLVM
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D56611/new/
>
> https://reviews.llvm.org/D56611
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Fix here: https://reviews.llvm.org/D57560


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

The complete commands that's failing is:

  /usr/bin/clang++-8  -fPIC -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -std=c++11 -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O0 -g3  -fuse-ld=gold -Wl,-allow-shlib-undefined 
tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o
 tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o  -o 
bin/clangd  -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.so.9svn -lpthread 
lib/libclangBasic.so.9svn lib/libclangTidy.so.9svn lib/libclangDaemon.so.9svn 
lib/libclangFormat.so.9svn lib/libclangFrontend.so.9svn 
lib/libclangSema.so.9svn lib/libclangTooling.so.9svn 
lib/libclangToolingCore.so.9svn


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56611#1379876 , @sammccall wrote:

> Hi Nathan,
>  What platform is this on? Not seeing it on the buildbots.
>  Anything unusual in build setup (standalone build, building with shared 
> libraries, etc)?


I'm on Linux, building with shared libraries. Not sure what standalone means in 
this context, I'm building the monorepo with `-DLLVM_ENABLE_PROJECTS="clang"`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56611#1379785 , @nridge wrote:

> This commit is causing clangd to fail to link, with errors like this:
>
>   
> tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o:SwapIfBranches.cpp:function
>  clang::RecursiveASTVisitor namespace)::FindIfUnderCursor>::TraverseParmVarDecl(clang::ParmVarDecl*): 
> error: undefined reference to 'clang::ParmVarDecl::hasDefaultArg() const'
>
>
> (and dozens more that are similar).


Hi Nathan,
What platform is this on? Not seeing it on the buildbots.
Anything unusual in build setup (standalone build, building with shared 
libraries, etc)?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.
Herald added a project: LLVM.

This commit is causing clangd to fail to link, with errors like this:

  
tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o:SwapIfBranches.cpp:function
 clang::RecursiveASTVisitor::TraverseParmVarDecl(clang::ParmVarDecl*): 
error: undefined reference to 'clang::ParmVarDecl::hasDefaultArg() const'

(and dozens more that are similar).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352796: [clangd] A code action to swap branches of an if 
statement (authored by ibiryukov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56611?vs=184588=184594#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/trunk/clangd/SourceCode.cpp
  clang-tools-extra/trunk/clangd/SourceCode.h
  clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/trunk/clangd/refactor/tweaks/Dummy.cpp
  clang-tools-extra/trunk/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/trunk/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/trunk/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -0,0 +1,132 @@
+//===--- SwapIfBranches.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 "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+/// Swaps the 'then' and 'else' branch of the if statement.
+/// Before:
+///   if (foo) { return 10; } else { continue; }
+///   ^^^ 
+/// After:
+///   if (foo) { continue; } else { return 10; }
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() const override final;
+
+  bool prepare(const Selection ) override;
+  Expected apply(const Selection ) override;
+  std::string title() const override;
+
+private:
+  IfStmt *If = nullptr;
+};
+
+REGISTER_TWEAK(SwapIfBranches);
+
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+// Check if the cursor is in the range of 'if (cond)'.
+// FIXME: this does not contain the closing paren, add it too.
+auto R = toHalfOpenFileRange(
+Ctx.getSourceManager(), Ctx.getLangOpts(),
+SourceRange(If->getIfLoc(), If->getCond()->getEndLoc().isValid()
+? If->getCond()->getEndLoc()
+: If->getIfLoc()));
+if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) {
+  Result = If;
+  return false;
+}
+// Check the range of 'else'.
+R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+SourceRange(If->getElseLoc()));
+if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) {
+  Result = If;
+  return false;
+}
+
+return true;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection ) {
+  auto  = Inputs.AST.getASTContext();
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!If->getThen() || !llvm::isa(If->getThen()) ||
+  !If->getElse() || !llvm::isa(If->getElse()))
+return false;
+  return true;
+}
+
+Expected SwapIfBranches::apply(const Selection ) {
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"Could not obtain range of the 'then' branch. Macros?");
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"Could not obtain range of the 'else' branch. 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 184588.
ilya-biryukov marked 8 inline comments as done.
ilya-biryukov added a comment.
Herald added a project: clang.
Herald added a subscriber: llvm-commits.

- Remove Dummy.cpp
- Add halfOpenRangeTouches
- Add a comment about file vs expansion locations
- Move range manipulations with else and then to apply()
- Remove test fixture, turn test member functions into free functions
- Add checkTransform
- Replace a null check for getCond() with an isValid() check for the 
corresponding location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/Dummy.cpp
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/unittests/clangd/TweakTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -0,0 +1,158 @@
+//===-- TweakTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Expr.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using llvm::Failed;
+using llvm::HasValue;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+  Annotations Code(Input);
+  ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+  << "no points of interest specified";
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.Code = Code.code();
+
+  ParsedAST AST = TU.build();
+
+  auto CheckOver = [&](Range Selection) {
+auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+AST.getASTContext().getSourceManager(), Selection.start));
+auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc});
+if (Available)
+  EXPECT_THAT_EXPECTED(T, Succeeded())
+  << "code is " << markRange(Code.code(), Selection);
+else
+  EXPECT_THAT_EXPECTED(T, Failed())
+  << "code is " << markRange(Code.code(), Selection);
+  };
+  for (auto P : Code.points())
+CheckOver(Range{P, P});
+  for (auto R : Code.ranges())
+CheckOver(R);
+}
+
+/// Checks action is available at every point and range marked in \p Input.
+void checkAvailable(TweakID ID, llvm::StringRef Input) {
+  return checkAvailable(ID, Input, /*Available=*/true);
+}
+
+/// Same as checkAvailable, but checks the action is not available.
+void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+  return checkAvailable(ID, Input, /*Available=*/false);
+}
+llvm::Expected apply(TweakID ID, llvm::StringRef Input) {
+  Annotations Code(Input);
+  Range SelectionRng;
+  if (Code.points().size() != 0) {
+assert(Code.ranges().size() == 0 &&
+   "both a cursor point and a selection range were specified");
+SelectionRng = Range{Code.point(), Code.point()};
+  } else {
+SelectionRng = Code.range();
+  }
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.Code = Code.code();
+
+  ParsedAST AST = TU.build();
+  auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+  AST.getASTContext().getSourceManager(), SelectionRng.start));
+  Tweak::Selection S = {Code.code(), AST, CursorLoc};
+
+  auto T = prepareTweak(ID, S);
+  if (!T)
+return T.takeError();
+  auto Replacements = (*T)->apply(S);
+  if (!Replacements)
+return Replacements.takeError();
+  return applyAllReplacements(Code.code(), *Replacements);
+}
+
+void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
+llvm::StringRef Output) {
+  

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.h:63
 
+/// Turns a token range into a half-open range and checks its correctness.
+/// The resulting range will have only valid source location on both sides, 
both

sammccall wrote:
> I think the semantics of the locations (expansion vs spelling) need to be 
> spelled out here, at least in the comment.
Added a small comment, but it could probably be improved. Let me know what's 
missing.



Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:60
+Ctx.getSourceManager(), Ctx.getLangOpts(),
+SourceRange(If->getIfLoc(), If->getCond() ? If->getCond()->getEndLoc()
+  : If->getIfLoc()));

sammccall wrote:
> what's the case where the condition is null?
I initially thought of invalid parse trees (i.e. `if() {}`), but it turns out 
this never happens. 

From experience, it's better to assume everything can be null in the AST. This 
saves a lot of time chasing rare null dereferences later and a lot of time 
investigating whether something is null in a particular case we're facing.

In our particular case `IfStmt` seems to always have a condition, but the 
condition will have an invalid location when it's empty. I've added a test that 
the action still works (I see no reason why it shouldn't if we assume the 
parser recovery is good). Happy to discuss and adjust, though, let me know what 
you think.



Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:63
+if (R && (halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc) ||
+  R->getEnd() == CursorLoc)) {
+  Result = If;

sammccall wrote:
> the explicit checks for equality at the endpoint seem... odd
> do you need a halfOpenRangeTouches() or so?
Done. Thanks for the suggestion, this does make the code much clearer.



Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:101
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());

sammccall wrote:
> do this in apply()?
> 
> This is an example refactoring, if we have to add a bunch of checks because 
> it fires too often then we should try to solve that systematically.
Done.

This goes back to a similar comment about precomputing the replacements... 
Doing this in advance means we're not showing the check in some cases where it 
would later fail because of macros.

However, macros support is poor in the current state of the checks anyway, 
we'll need to invest some more time to make it better.



Comment at: clang-tools-extra/unittests/clangd/TweakTests.cpp:116
+TEST_F(TweakTest, SwapIfBranches) {
+  llvm::StringLiteral ID = "SwapIfBranches";
+

sammccall wrote:
> why StringLiteral here?
Seems like a good default for a constant string, i.e. it can compute the size 
at compile time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/SourceCode.h:63
 
+/// Turns a token range into a half-open range and checks its correctness.
+/// The resulting range will have only valid source location on both sides, 
both

I think the semantics of the locations (expansion vs spelling) need to be 
spelled out here, at least in the comment.



Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:60
+Ctx.getSourceManager(), Ctx.getLangOpts(),
+SourceRange(If->getIfLoc(), If->getCond() ? If->getCond()->getEndLoc()
+  : If->getIfLoc()));

what's the case where the condition is null?



Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:63
+if (R && (halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc) ||
+  R->getEnd() == CursorLoc)) {
+  Result = If;

the explicit checks for equality at the endpoint seem... odd
do you need a halfOpenRangeTouches() or so?



Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:101
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());

do this in apply()?

This is an example refactoring, if we have to add a bunch of checks because it 
fires too often then we should try to solve that systematically.



Comment at: clang-tools-extra/unittests/clangd/TweakTests.cpp:46
+
+class TweakTest : public ::testing::Test {
+public:

fixture has no state, can these be free functions?



Comment at: clang-tools-extra/unittests/clangd/TweakTests.cpp:116
+TEST_F(TweakTest, SwapIfBranches) {
+  llvm::StringLiteral ID = "SwapIfBranches";
+

why StringLiteral here?



Comment at: clang-tools-extra/unittests/clangd/TweakTests.cpp:140
+  )cpp";
+  EXPECT_THAT_EXPECTED(apply(ID, Input), HasValue(Output));
+}

`checkTransform(ID, Input, Output)`, to avoid mixing styles?



Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:50
+
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:

ilya-biryukov wrote:
> sammccall wrote:
> > (Mostly this is food for thought for later - we shouldn't try to solve 
> > everything in this patch)
> > 
> > Two efficiency problems here:
> >  - doing one traversal per tweak is wasteful (when we have >1 tweaks, but 
> > worth at least *thinking* about)
> >  - traversing the entire AST rather than just the nodes over the cursor is 
> > bad news (though "over the cursor" may not be trivial to define)
> > 
> > Idea for how to "framework" this problem away:
> > Add `vector SelectedNodes` to the inputs, the idea being that 
> > this is the stack from the narrowest `Expr` under the cursor to the 
> > `TranslationUnitDecl` at the top.
> > This can be produced by a RecursiveASTVisitor (that either traverses the 
> > whole AST, or just the bits under the cursor if that's simple enough).
> > Tweaks would iterate over the nodes from narrowest to broadest, deciding 
> > whether to select this node for processing, continue iterating, or bail out.
> > 
> > Matching in checks are then pretty easy to write, we haven't removed too 
> > much flexibility in flow control, and it's pretty hard to write a slow 
> > check.
> > 
> > There are some complications:
> >  - we need access to parents somehow (e.g. consider the case of adding NS 
> > qualifiers, we want to match on names but then traverse up to the 
> > containing declrefexpr to get the nested namespace loc)
> >  - the interesting nodes may be a tree rather than a stack, because nodes 
> > overlap in the source. We could store a postorder traversal... but this 
> > makes e.g. "bail out when you hit a functiondecl" harder - you probably 
> > want to bail out of the current branch only.
> >  - things get complicated when we think about macros - depending on the 
> > semantics we want, it may be hard for the framework to prune parts of the 
> > tree that aren't under the cursor withouth traversing the whole AST.
> I mostly agree, however I still wonder whether that would be inefficient in 
> some cases. E.g. consider a corner if the input selection from LSP contains 
> the whole file? I see two options there: (1) putting all nodes of a file into 
> `vector`, (2) putting only the top-level `TranslationUnitDecl` 
> there.
> 
> It seems easy to end up with (1) to make the interface useful, however we 
> probably prefer (2) because otherwise we're back into the worst-case 
> scenario, i.e. "every  tweak traverses all the nodes"
> 
> > we need access to parents somehow (e.g. consider the case of adding NS 
> > qualifiers, we want to match on 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 184041.
ilya-biryukov added a comment.

- Update the license header


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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/unittests/clangd/TweakTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -0,0 +1,145 @@
+//===-- TweakTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Expr.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using llvm::Failed;
+using llvm::HasValue;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+class TweakTest : public ::testing::Test {
+public:
+  /// Checks action is available at every point and range marked in \p Input.
+  void checkAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  /// Same as checkAvailable, but checks the action is not available.
+  void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/false);
+  }
+
+  llvm::Expected apply(TweakID ID, llvm::StringRef Input) {
+Annotations Code(Input);
+Range SelectionRng;
+if (Code.points().size() != 0) {
+  assert(Code.ranges().size() == 0 &&
+ "both a cursor point and a selection range were specified");
+  SelectionRng = Range{Code.point(), Code.point()};
+} else {
+  SelectionRng = Code.range();
+}
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+AST.getASTContext().getSourceManager(), SelectionRng.start));
+Tweak::Selection S = {Code.code(), AST, CursorLoc};
+
+auto T = prepareTweak(ID, S);
+if (!T)
+  return T.takeError();
+auto Replacements = (*T)->apply(S);
+if (!Replacements)
+  return Replacements.takeError();
+return applyAllReplacements(Code.code(), *Replacements);
+  }
+
+private:
+  void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+Annotations Code(Input);
+ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+<< "no points of interest specified";
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+
+auto CheckOver = [&](Range Selection) {
+  auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+  AST.getASTContext().getSourceManager(), Selection.start));
+  auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc});
+  if (Available)
+EXPECT_THAT_EXPECTED(T, Succeeded())
+<< "code is " << markRange(Code.code(), Selection);
+  else
+EXPECT_THAT_EXPECTED(T, Failed())
+<< "code is " << markRange(Code.code(), Selection);
+};
+for (auto P : Code.points())
+  CheckOver(Range{P, P});
+for (auto R : Code.ranges())
+  CheckOver(R);
+  }
+};
+
+TEST_F(TweakTest, SwapIfBranches) {
+  llvm::StringLiteral ID = "SwapIfBranches";
+
+  checkAvailable(ID, R"cpp(
+void test() {
+  ^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; }
+}
+  )cpp");
+
+  checkNotAvailable(ID, R"cpp(
+void test() {
+  if (true) {^return ^100;^ } else { ^continue^;^ }
+}
+  )cpp");
+
+  llvm::StringLiteral Input = R"cpp(
+void test() {
+  ^if (true) { return 100; } else { continue; }
+}
+  )cpp";

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:34
+/// After:
+///   if (foo) { continue; } else { return 10; }
+class SwapIfBranches : public Tweak {

sammccall wrote:
> The before/after is useful, we should probably have it for all tweaks if 
> possible.
> It'd also be useful to notate where the cursor can be to trigger the action. 
> Partly because it forces us to consider this!
> 
> e.g. (not sure if this actually matches the logic you want, just an example)
> ```
> Before:
>   if (foo) { return 10; } else { continue; }
>   ^^   ^
> After:
>   ...
> ```
LG, updated the comment and ranges per suggestion.
I still not sure if the action should span the whole condition, but going with 
the suggested ranges anyway. We can tweak them later if the resulting ranges 
turn out to be problematic.

However, note that I excluded `{` and `}` from available ranges, but only 
because I'm lazy and this requires declaring some more local vars, etc.



Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:46
+private:
+  tooling::Replacements Result;
+};

sammccall wrote:
> I think prepare() should just verify:
>  - cursor is in the right place
>  - else statement exists and isn't an else if
>  - both branches are compound statements (for now)
>  - (maybe) relevant locations (if keyword, else keyword, braces) are not 
> macro expansions
> and then record the relevant source locations (or just the IfStmt*)
> 
> We may be able to get away with doing all the work in `prepare()`, but it's 
> not a good model for future tweaks. (And it is at least somewhat wasteful on 
> a hot path).
Done.
The only reason I thought applying replacements was a sensible idea is because 
it would fail in some weird macro cases (only when resulting ranges overlap, 
though). And it seemed better to avoid showing the action rather than showing 
it and failing later.



Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:50
+
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:

sammccall wrote:
> (Mostly this is food for thought for later - we shouldn't try to solve 
> everything in this patch)
> 
> Two efficiency problems here:
>  - doing one traversal per tweak is wasteful (when we have >1 tweaks, but 
> worth at least *thinking* about)
>  - traversing the entire AST rather than just the nodes over the cursor is 
> bad news (though "over the cursor" may not be trivial to define)
> 
> Idea for how to "framework" this problem away:
> Add `vector SelectedNodes` to the inputs, the idea being that 
> this is the stack from the narrowest `Expr` under the cursor to the 
> `TranslationUnitDecl` at the top.
> This can be produced by a RecursiveASTVisitor (that either traverses the 
> whole AST, or just the bits under the cursor if that's simple enough).
> Tweaks would iterate over the nodes from narrowest to broadest, deciding 
> whether to select this node for processing, continue iterating, or bail out.
> 
> Matching in checks are then pretty easy to write, we haven't removed too much 
> flexibility in flow control, and it's pretty hard to write a slow check.
> 
> There are some complications:
>  - we need access to parents somehow (e.g. consider the case of adding NS 
> qualifiers, we want to match on names but then traverse up to the containing 
> declrefexpr to get the nested namespace loc)
>  - the interesting nodes may be a tree rather than a stack, because nodes 
> overlap in the source. We could store a postorder traversal... but this makes 
> e.g. "bail out when you hit a functiondecl" harder - you probably want to 
> bail out of the current branch only.
>  - things get complicated when we think about macros - depending on the 
> semantics we want, it may be hard for the framework to prune parts of the 
> tree that aren't under the cursor withouth traversing the whole AST.
I mostly agree, however I still wonder whether that would be inefficient in 
some cases. E.g. consider a corner if the input selection from LSP contains the 
whole file? I see two options there: (1) putting all nodes of a file into 
`vector`, (2) putting only the top-level `TranslationUnitDecl` 
there.

It seems easy to end up with (1) to make the interface useful, however we 
probably prefer (2) because otherwise we're back into the worst-case scenario, 
i.e. "every  tweak traverses all the nodes"

> we need access to parents somehow (e.g. consider the case of adding NS 
> qualifiers, we want to match on names but then traverse up to the containing 
> declrefexpr to get the nested namespace loc)

I wonder if it's possible to instead write the checks so that they only look at 
the children of the nodes in a vector? My bet is that almost all of the checks 
only need to look one or two levels down, so this shouldn't turn into 
inefficiency.

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182910.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.

- Fix a typo in a comment: isValidRange -> isValidFileRange
- Make action available under 'else' keywords and conditions
- Move the logic of creating replacements to apply
- Use llvm::isa instead of dyn_cast_or_null


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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/unittests/clangd/TweakTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -0,0 +1,145 @@
+//===-- TweakTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Expr.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using llvm::Failed;
+using llvm::HasValue;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+class TweakTest : public ::testing::Test {
+public:
+  /// Checks action is available at every point and range marked in \p Input.
+  void checkAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  /// Same as checkAvailable, but checks the action is not available.
+  void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/false);
+  }
+
+  llvm::Expected apply(TweakID ID, llvm::StringRef Input) {
+Annotations Code(Input);
+Range SelectionRng;
+if (Code.points().size() != 0) {
+  assert(Code.ranges().size() == 0 &&
+ "both a cursor point and a selection range were specified");
+  SelectionRng = Range{Code.point(), Code.point()};
+} else {
+  SelectionRng = Code.range();
+}
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+AST.getASTContext().getSourceManager(), SelectionRng.start));
+Tweak::Selection S = {Code.code(), AST, CursorLoc};
+
+auto T = prepareTweak(ID, S);
+if (!T)
+  return T.takeError();
+auto Replacements = (*T)->apply(S);
+if (!Replacements)
+  return Replacements.takeError();
+return applyAllReplacements(Code.code(), *Replacements);
+  }
+
+private:
+  void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+Annotations Code(Input);
+ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+<< "no points of interest specified";
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+
+auto CheckOver = [&](Range Selection) {
+  auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+  AST.getASTContext().getSourceManager(), Selection.start));
+  auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc});
+  if (Available)
+EXPECT_THAT_EXPECTED(T, Succeeded())
+<< "code is " << markRange(Code.code(), Selection);
+  else
+EXPECT_THAT_EXPECTED(T, Failed())
+<< "code is " << markRange(Code.code(), Selection);
+};
+for (auto P : Code.points())
+  CheckOver(Range{P, P});
+for (auto R : Code.ranges())
+  CheckOver(R);
+  }
+};
+
+TEST_F(TweakTest, SwapIfBranches) {
+  llvm::StringLiteral ID = "SwapIfBranches";
+
+  checkAvailable(ID, R"cpp(
+void test() {
+  ^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; }
+}
+  )cpp");
+
+  

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182900.
ilya-biryukov added a comment.

- Use a helper to avoid creating RewriteBuffer


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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/unittests/clangd/TweakTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -0,0 +1,145 @@
+//===-- TweakTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Expr.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using llvm::Failed;
+using llvm::HasValue;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+class TweakTest : public ::testing::Test {
+public:
+  /// Checks action is available at every point and range marked in \p Input.
+  void checkAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  /// Same as checkAvailable, but checks the action is not available.
+  void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/false);
+  }
+
+  llvm::Expected apply(TweakID ID, llvm::StringRef Input) {
+Annotations Code(Input);
+Range SelectionRng;
+if (Code.points().size() != 0) {
+  assert(Code.ranges().size() == 0 &&
+ "both a cursor point and a selection range were specified");
+  SelectionRng = Range{Code.point(), Code.point()};
+} else {
+  SelectionRng = Code.range();
+}
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+AST.getASTContext().getSourceManager(), SelectionRng.start));
+Tweak::Selection S = {Code.code(), AST, CursorLoc};
+
+auto T = prepareTweak(ID, S);
+if (!T)
+  return T.takeError();
+auto Replacements = (*T)->apply(S);
+if (!Replacements)
+  return Replacements.takeError();
+return applyAllReplacements(Code.code(), *Replacements);
+  }
+
+private:
+  void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+Annotations Code(Input);
+ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+<< "no points of interest specified";
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+
+auto CheckOver = [&](Range Selection) {
+  auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+  AST.getASTContext().getSourceManager(), Selection.start));
+  auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc});
+  if (Available)
+EXPECT_THAT_EXPECTED(T, Succeeded())
+<< "code is " << markRange(Code.code(), Selection);
+  else
+EXPECT_THAT_EXPECTED(T, Failed())
+<< "code is " << markRange(Code.code(), Selection);
+};
+for (auto P : Code.points())
+  CheckOver(Range{P, P});
+for (auto R : Code.ranges())
+  CheckOver(R);
+  }
+};
+
+TEST_F(TweakTest, SwapIfBranches) {
+  llvm::StringLiteral ID = "SwapIfBranches";
+
+  checkAvailable(ID, R"cpp(
+void test() {
+  ^i^f (true) { return 100; } else { continue; }
+}
+  )cpp");
+
+  checkNotAvailable(ID, R"cpp(
+void test() {
+  if ^(^true) { return 100; } else { continue; }
+}
+  )cpp");
+
+  llvm::StringLiteral Input = R"cpp(
+void test() {
+  ^if (true) { return 100; } else { continue; }
+}
+  

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182884.
ilya-biryukov added a comment.

- Use helper to avoid creating RewriteBuffer
- Use std::string to fix stack corruption


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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/unittests/clangd/TweakTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -0,0 +1,145 @@
+//===-- TweakTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Expr.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using llvm::Failed;
+using llvm::HasValue;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+class TweakTest : public ::testing::Test {
+public:
+  /// Checks action is available at every point and range marked in \p Input.
+  void checkAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  /// Same as checkAvailable, but checks the action is not available.
+  void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  llvm::Expected apply(TweakID ID, llvm::StringRef Input) {
+Annotations Code(Input);
+Range SelectionRng;
+if (Code.points().size() != 0) {
+  assert(Code.ranges().size() == 0 &&
+ "both a cursor point and a selection range were specified");
+  SelectionRng = Range{Code.point(), Code.point()};
+} else {
+  SelectionRng = Code.range();
+}
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+AST.getASTContext().getSourceManager(), SelectionRng.start));
+Tweak::Selection S = {Code.code(), AST, CursorLoc};
+
+auto T = prepareTweak(ID, S);
+if (!T)
+  return T.takeError();
+auto Replacements = (*T)->apply(S);
+if (!Replacements)
+  return Replacements.takeError();
+return applyAllReplacements(Code.code(), *Replacements);
+  }
+
+private:
+  void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+Annotations Code(Input);
+ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+<< "no points of interest specified";
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+
+auto CheckOver = [&](Range Selection) {
+  auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+  AST.getASTContext().getSourceManager(), Selection.start));
+  auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc});
+  if (Available)
+EXPECT_THAT_EXPECTED(T, Failed())
+<< "code is " << markRange(Code.code(), Selection);
+  else
+EXPECT_THAT_EXPECTED(T, Succeeded())
+<< "code is " << markRange(Code.code(), Selection);
+};
+for (auto P : Code.points())
+  CheckOver(Range{P, P});
+for (auto R : Code.ranges())
+  CheckOver(R);
+  }
+};
+
+TEST_F(TweakTest, SwapIfBranches) {
+  llvm::StringLiteral ID = "SwapIfBranches";
+
+  checkAvailable(ID, R"cpp(
+void test() {
+  ^i^f (true) { return 100; } else { continue; }
+}
+  )cpp");
+
+  checkNotAvailable(ID, R"cpp(
+void test() {
+  if ^(^true) { return 100; } else { continue; }
+}
+  )cpp");
+
+  llvm::StringLiteral Input = R"cpp(
+void test() {
+  ^if (true) { 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182872.
ilya-biryukov added a comment.

- Update id of swap branches in tests
- Rebase


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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/unittests/clangd/TweakTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -0,0 +1,156 @@
+//===-- TweakTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Expr.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using llvm::Failed;
+using llvm::HasValue;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+class TweakTest : public ::testing::Test {
+public:
+  /// Checks action is available at every point and range marked in \p Input.
+  void checkAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  /// Same as checkAvailable, but checks the action is not available.
+  void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  llvm::Expected apply(TweakID ID, llvm::StringRef Input) {
+Annotations Code(Input);
+Range SelectionRng;
+if (Code.points().size() != 0) {
+  assert(Code.ranges().size() == 0 &&
+ "both a cursor point and a selection range were specified");
+  SelectionRng = Range{Code.point(), Code.point()};
+} else {
+  SelectionRng = Code.range();
+}
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+AST.getASTContext().getSourceManager(), SelectionRng.start));
+Tweak::Selection S = {Code.code(), AST, CursorLoc};
+
+auto T = prepareTweak(ID, S);
+if (!T)
+  return T.takeError();
+auto Replacements = (*T)->apply(S);
+if (!Replacements)
+  return Replacements.takeError();
+Rewriter RW(AST.getASTContext().getSourceManager(),
+AST.getASTContext().getLangOpts());
+bool Succeeded = tooling::applyAllReplacements(*Replacements, RW);
+(void)(bool) Succeeded;
+assert(Succeeded && "failed to apply replacements");
+
+std::string Result;
+llvm::raw_string_ostream OS(Result);
+RW.getRewriteBufferFor(
+  AST.getASTContext().getSourceManager().getMainFileID())
+->write(OS);
+return OS.str();
+  }
+
+private:
+  void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+Annotations Code(Input);
+ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+<< "no points of interest specified";
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+
+auto CheckOver = [&](Range Selection) {
+  auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+  AST.getASTContext().getSourceManager(), Selection.start));
+  auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc});
+  if (Available)
+EXPECT_THAT_EXPECTED(T, Failed())
+<< "code is " << markRange(Code.code(), Selection);
+  else
+EXPECT_THAT_EXPECTED(T, Succeeded())
+<< "code is " << markRange(Code.code(), Selection);
+};
+for (auto P : Code.points())
+  CheckOver(Range{P, P});
+for (auto R : Code.ranges())
+  CheckOver(R);
+  }
+};
+
+TEST_F(TweakTest, SwapIfBranches) {
+ 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182555.
ilya-biryukov added a comment.

- Add tests.


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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/TweakTests.cpp

Index: clang-tools-extra/unittests/clangd/TweakTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -0,0 +1,148 @@
+//===-- TweakTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Expr.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using llvm::Failed;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+class TweakTest : public ::testing::Test {
+public:
+  /// Checks action is available at every point and range marked in \p Input.
+  void checkAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  /// Same as checkAvailable, but checks the action is not available.
+  void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+return checkAvailable(ID, Input, /*Available=*/true);
+  }
+
+  llvm::StringRef apply(TweakID ID, llvm::StringRef Input) {
+Annotations Code(Input);
+Range SelectionRng;
+if (Code.points().size() != 0) {
+  assert(Code.ranges().size() == 0 &&
+ "both a cursor point and a selection range were specified");
+  SelectionRng = Range{Code.point(), Code.point()};
+} else {
+  SelectionRng = Code.range();
+}
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+auto Selection =
+llvm::cantFail(Tweak::Selection::create(Code.code(), AST, SelectionRng));
+auto T = llvm::cantFail(prepareTweak(ID, Selection));
+auto Replacements = llvm::cantFail(T->apply(Selection));
+Rewriter RW(AST.getASTContext().getSourceManager(),
+AST.getASTContext().getLangOpts());
+bool Succeeded = tooling::applyAllReplacements(Replacements, RW);
+(void)(bool) Succeeded;
+assert(Succeeded && "failed to apply replacements");
+
+std::string Result;
+llvm::raw_string_ostream OS(Result);
+RW.getRewriteBufferFor(
+  AST.getASTContext().getSourceManager().getMainFileID())
+->write(OS);
+return OS.str();
+  }
+
+private:
+  void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+Annotations Code(Input);
+ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+<< "no points of interest specified";
+TestTU TU;
+TU.Filename = "foo.cpp";
+TU.Code = Code.code();
+
+ParsedAST AST = TU.build();
+
+auto CheckOver = [&](Range Selection) {
+  auto T = prepareTweak(
+  ID, cantFail(Tweak::Selection::create(Code.code(), AST, Selection)));
+  if (Available)
+EXPECT_THAT_EXPECTED(T, Failed())
+<< "code is " << markRange(Code.code(), Selection);
+  else
+EXPECT_THAT_EXPECTED(T, Succeeded())
+<< "code is " << markRange(Code.code(), Selection);
+};
+for (auto P : Code.points())
+  CheckOver(Range{P, P});
+for (auto R : Code.ranges())
+  CheckOver(R);
+  }
+};
+
+TEST_F(TweakTest, SwapIfBranches) {
+  TweakID ID = "swap-if-branches";
+
+  checkAvailable(ID, R"cpp(
+void test() {
+  ^i^f (true) { return 100; } else { continue; }
+}
+  )cpp");
+
+  checkNotAvailable(ID, R"cpp(
+void test() {
+  if ^(^true) { return 100; } else { continue; }
+}
+  )cpp");
+
+  llvm::StringLiteral 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182546.
ilya-biryukov added a comment.

- Move source code helpers to this patch


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

https://reviews.llvm.org/D56611

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp

Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -0,0 +1,119 @@
+//===--- SwapIfBranches.cpp --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+/// Swaps the 'then' and 'else' branch of the if statement.
+/// Before:
+///   if (foo) { return 10; } else { continue; }
+/// After:
+///   if (foo) { continue; } else { return 10; }
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() const override {
+return llvm::StringLiteral("swap-if-branches");
+  }
+
+  bool prepare(const Selection ) override;
+  Expected apply(const Selection ) override;
+  std::string title() const override;
+
+private:
+  tooling::Replacements Result;
+};
+REGISTER_TWEAK(SwapIfBranches);
+
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+ SourceRange(If->getIfLoc()));
+if (!R)
+  return true;
+if (!halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc))
+  return true;
+Result = If;
+return false;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection ) {
+
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+  IfStmt *If = nullptr;
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))
+return false;
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return false;
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return false;
+
+  llvm::StringRef ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  llvm::StringRef ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ThenRng->getBegin(),
+ ThenCode.size(), ElseCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ElseRng->getBegin(),
+ ElseCode.size(), ThenCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  return true;
+}
+
+Expected SwapIfBranches::apply(const Selection ) {
+  return Result;
+}
+
+std::string SwapIfBranches::title() const { return "Swap if branches"; }
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -9,4 +9,5 @@
 # clangd/tool/CMakeLists.txt for an example.
 add_clang_library(clangDaemonTweaks OBJECT
   QualifyName.cpp
+  SwapIfBranches.cpp
   )
Index: clang-tools-extra/clangd/SourceCode.h
===
--- 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Cool! Adopting this one as the simplest tweak for "how should tweaks do X" 
questions.

This depends on helpers not (yet) present in this patch, so not commenting on 
them now.

Need unit tests for tweaks. Something like this should work:

  Annotations Test(R"cpp(void foo() {
[[if]] (1) { return; } else { continue; }
  })cpp");
  auto AST = TestTU(Test.code()).build();
  auto Sel = Tweak::Selection::create(Test.Code(), AST, Test.range());
  auto T = prepareTweak("SwapIfBranches", Sel);
  ASSERT_TRUE(T) << T.takeError();
  auto Edits = T.apply(Sel);
  ASSERT_TRUE(Edits) << Edits.takeError();
  auto After = applyAllReplacements(Test.code(), *Edits);
  EXPECT_EQ(After, R"cpp(void foo() {
if (1) { continue; } else { return; }
  })cpp");

Probably want to wrap it up into a table driven test once we have a few.




Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:34
+/// After:
+///   if (foo) { continue; } else { return 10; }
+class SwapIfBranches : public Tweak {

The before/after is useful, we should probably have it for all tweaks if 
possible.
It'd also be useful to notate where the cursor can be to trigger the action. 
Partly because it forces us to consider this!

e.g. (not sure if this actually matches the logic you want, just an example)
```
Before:
  if (foo) { return 10; } else { continue; }
  ^^   ^
After:
  ...
```



Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:46
+private:
+  tooling::Replacements Result;
+};

I think prepare() should just verify:
 - cursor is in the right place
 - else statement exists and isn't an else if
 - both branches are compound statements (for now)
 - (maybe) relevant locations (if keyword, else keyword, braces) are not macro 
expansions
and then record the relevant source locations (or just the IfStmt*)

We may be able to get away with doing all the work in `prepare()`, but it's not 
a good model for future tweaks. (And it is at least somewhat wasteful on a hot 
path).



Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:50
+
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:

(Mostly this is food for thought for later - we shouldn't try to solve 
everything in this patch)

Two efficiency problems here:
 - doing one traversal per tweak is wasteful (when we have >1 tweaks, but worth 
at least *thinking* about)
 - traversing the entire AST rather than just the nodes over the cursor is bad 
news (though "over the cursor" may not be trivial to define)

Idea for how to "framework" this problem away:
Add `vector SelectedNodes` to the inputs, the idea being that 
this is the stack from the narrowest `Expr` under the cursor to the 
`TranslationUnitDecl` at the top.
This can be produced by a RecursiveASTVisitor (that either traverses the whole 
AST, or just the bits under the cursor if that's simple enough).
Tweaks would iterate over the nodes from narrowest to broadest, deciding 
whether to select this node for processing, continue iterating, or bail out.

Matching in checks are then pretty easy to write, we haven't removed too much 
flexibility in flow control, and it's pretty hard to write a slow check.

There are some complications:
 - we need access to parents somehow (e.g. consider the case of adding NS 
qualifiers, we want to match on names but then traverse up to the containing 
declrefexpr to get the nested namespace loc)
 - the interesting nodes may be a tree rather than a stack, because nodes 
overlap in the source. We could store a postorder traversal... but this makes 
e.g. "bail out when you hit a functiondecl" harder - you probably want to bail 
out of the current branch only.
 - things get complicated when we think about macros - depending on the 
semantics we want, it may be hard for the framework to prune parts of the tree 
that aren't under the cursor withouth traversing the whole AST.



Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:84
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))

isa


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182321.
ilya-biryukov added a comment.

- Remove the header file


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611

Files:
  clangd/refactor/tweaks/CMakeLists.txt
  clangd/refactor/tweaks/SwapIfBranches.cpp

Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -0,0 +1,119 @@
+//===--- SwapIfBranches.cpp --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+/// Swaps the 'then' and 'else' branch of the if statement.
+/// Before:
+///   if (foo) { return 10; } else { continue; }
+/// After:
+///   if (foo) { continue; } else { return 10; }
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() const override {
+return llvm::StringLiteral("swap-if-branches");
+  }
+
+  bool prepare(const Selection ) override;
+  Expected apply(const Selection ) override;
+  std::string title() const override;
+
+private:
+  tooling::Replacements Result;
+};
+REGISTER_TWEAK(SwapIfBranches);
+
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+ SourceRange(If->getIfLoc()));
+if (!R)
+  return true;
+if (!halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc))
+  return true;
+Result = If;
+return false;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection ) {
+
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+  IfStmt *If = nullptr;
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))
+return false;
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return false;
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return false;
+
+  llvm::StringRef ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  llvm::StringRef ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ThenRng->getBegin(),
+ ThenCode.size(), ElseCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ElseRng->getBegin(),
+ ElseCode.size(), ThenCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  return true;
+}
+
+Expected SwapIfBranches::apply(const Selection ) {
+  return Result;
+}
+
+std::string SwapIfBranches::title() const { return "Swap if branches"; }
+} // namespace clangd
+} // namespace clang
Index: clangd/refactor/tweaks/CMakeLists.txt
===
--- clangd/refactor/tweaks/CMakeLists.txt
+++ clangd/refactor/tweaks/CMakeLists.txt
@@ -9,4 +9,5 @@
 # clangd/tool/CMakeLists.txt for an example.
 add_clang_library(clangDaemonTweaks OBJECT
   QualifyName.cpp
+  SwapIfBranches.cpp
   )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182317.
ilya-biryukov added a comment.

- Update to reflect changes in parent revision


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611

Files:
  clangd/refactor/tweaks/CMakeLists.txt
  clangd/refactor/tweaks/SwapIfBranches.cpp
  clangd/refactor/tweaks/SwapIfBranches.h

Index: clangd/refactor/tweaks/SwapIfBranches.h
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.h
@@ -0,0 +1,41 @@
+//===--- SwapIfBrances.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Swaps the 'then' and 'else' branch of the if statement.
+// Before:
+//   if (foo) { return 10; } else { continue; }
+// After:
+//   if (foo) { continue; } else { return 10; }
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+
+#include "refactor/Tweak.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace clangd {
+
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() const override { return llvm::StringLiteral("swap-if-branches"); }
+
+  bool prepare(const Selection ) override;
+  Expected apply(const Selection ) override;
+  std::string title() const override;
+
+private:
+  tooling::Replacements Result;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -0,0 +1,102 @@
+//===--- SwapIfBranches.cpp --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "SwapIfBranches.h"
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+REGISTER_TWEAK(SwapIfBranches);
+
+namespace {
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+ SourceRange(If->getIfLoc()));
+if (!R)
+  return true;
+if (!halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc))
+  return true;
+Result = If;
+return false;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection ) {
+
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+  IfStmt *If = nullptr;
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))
+return false;
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return false;
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return false;
+
+  llvm::StringRef ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  llvm::StringRef ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ThenRng->getBegin(),
+ ThenCode.size(), ElseCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ElseRng->getBegin(),
+ 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182274.
ilya-biryukov added a comment.

- Rebase after parent change


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611

Files:
  clangd/refactor/tweaks/CMakeLists.txt
  clangd/refactor/tweaks/SwapIfBranches.cpp
  clangd/refactor/tweaks/SwapIfBranches.h

Index: clangd/refactor/tweaks/SwapIfBranches.h
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.h
@@ -0,0 +1,41 @@
+//===--- SwapIfBrances.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Swaps the 'then' and 'else' branch of the if statement.
+// Before:
+//   if (foo) { return 10; } else { continue; }
+// After:
+//   if (foo) { continue; } else { return 10; }
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+
+#include "refactor/Tweak.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace clangd {
+
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() override { return llvm::StringLiteral("swap-if-branches"); }
+
+  bool prepare(const Selection ) override;
+  Expected apply(const Selection ) override;
+  std::string title() const override;
+
+private:
+  tooling::Replacements Result;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -0,0 +1,102 @@
+//===--- SwapIfBranches.cpp --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "SwapIfBranches.h"
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+REGISTER_TWEAK(SwapIfBranches);
+
+namespace {
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+ SourceRange(If->getIfLoc()));
+if (!R)
+  return true;
+if (!halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc))
+  return true;
+Result = If;
+return false;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection ) {
+
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+  IfStmt *If = nullptr;
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))
+return false;
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return false;
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return false;
+
+  llvm::StringRef ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  llvm::StringRef ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ThenRng->getBegin(),
+ ThenCode.size(), ElseCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ElseRng->getBegin(),
+ 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182102.
ilya-biryukov added a comment.

- Fix a typo in the id of the SwapIfBranches


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611

Files:
  clangd/CMakeLists.txt
  clangd/refactor/tweaks/SwapIfBranches.cpp
  clangd/refactor/tweaks/SwapIfBranches.h

Index: clangd/refactor/tweaks/SwapIfBranches.h
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.h
@@ -0,0 +1,41 @@
+//===--- SwapIfBrances.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Swaps the 'then' and 'else' branch of the if statement.
+// Before:
+//   if (foo) { return 10; } else { continue; }
+// After:
+//   if (foo) { continue; } else { return 10; }
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+
+#include "refactor/Tweak.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace clangd {
+
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() override { return llvm::StringLiteral("swap-if-branches"); }
+
+  bool prepare(const Selection ) override;
+  Expected apply(const Selection ) override;
+  std::string title() const override;
+
+private:
+  tooling::Replacements Result;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -0,0 +1,102 @@
+//===--- SwapIfBranches.cpp --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "SwapIfBranches.h"
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+REGISTER_TWEAK(SwapIfBranches);
+
+namespace {
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+ SourceRange(If->getIfLoc()));
+if (!R)
+  return true;
+if (!halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc))
+  return true;
+Result = If;
+return false;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection ) {
+
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+  IfStmt *If = nullptr;
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))
+return false;
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return false;
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return false;
+
+  llvm::StringRef ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  llvm::StringRef ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ThenRng->getBegin(),
+ ThenCode.size(), ElseCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ElseRng->getBegin(),
+ 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 182080.
ilya-biryukov added a comment.

- Update after changes to parent revision


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611

Files:
  clangd/CMakeLists.txt
  clangd/refactor/tweaks/SwapIfBranches.cpp
  clangd/refactor/tweaks/SwapIfBranches.h

Index: clangd/refactor/tweaks/SwapIfBranches.h
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.h
@@ -0,0 +1,41 @@
+//===--- SwapIfBrances.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Swaps the 'then' and 'else' branch of the if statement.
+// Before:
+//   if (foo) { return 10; } else { continue; }
+// After:
+//   if (foo) { continue; } else { return 10; }
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_TWEAKS_SWAPIFBRANCHES_H
+
+#include "refactor/Tweak.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace clangd {
+
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() override { return llvm::StringLiteral("swap-if-brances"); }
+
+  bool prepare(const Selection ) override;
+  Expected apply(const Selection ) override;
+  std::string title() const override;
+
+private:
+  tooling::Replacements Result;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- /dev/null
+++ clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -0,0 +1,102 @@
+//===--- SwapIfBranches.cpp --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "SwapIfBranches.h"
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+REGISTER_TWEAK(SwapIfBranches);
+
+namespace {
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+ SourceRange(If->getIfLoc()));
+if (!R)
+  return true;
+if (!halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc))
+  return true;
+Result = If;
+return false;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection ) {
+
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+  IfStmt *If = nullptr;
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))
+return false;
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return false;
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return false;
+
+  llvm::StringRef ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  llvm::StringRef ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ThenRng->getBegin(),
+ ThenCode.size(), ElseCode))) {
+llvm::consumeError(std::move(Err));
+return false;
+  }
+  if (auto Err = Result.add(tooling::Replacement(SrcMgr, ElseRng->getBegin(),
+ 

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/refactor/actions/SwapIfBranches.cpp:36
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),

jkorous wrote:
> It seems to me we don't find If token whenever
> CursorLoc == location of 'f' of the If token
> 
> For example if there's "if" starting at location 1:1 I think we proceed like 
> this (hope my pseudocode is clear):
> 
> 1. `If->getIfLoc()` returns `SourceLocation{1:1}`
> 2. we construct `SourceRange{begin = 1:1, end = 1:1}`
> 3. `toHalfOpenFileRange()` returns `SourceRange{1:1, 1:2}`
> 4. the condition for `SourceLocation L` in `halfOpenRangeContains()` is `1:1 
> <= LOffset && LOffset < 1:2` which is only ever true for L == 1:1
> 
> Do I understand it right?
In step (3) the constructed range is intended to be `SourceRange{1:1, 1:3}`, 
i.e. it should cover both chars of the `if` keyword.
I haven't checked it, though, will make sure that's the case when actually 
testing this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Ilya, this seems really useful for people learning how to implement their 
custom actions!




Comment at: clangd/refactor/actions/SwapIfBranches.cpp:36
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),

It seems to me we don't find If token whenever
CursorLoc == location of 'f' of the If token

For example if there's "if" starting at location 1:1 I think we proceed like 
this (hope my pseudocode is clear):

1. `If->getIfLoc()` returns `SourceLocation{1:1}`
2. we construct `SourceRange{begin = 1:1, end = 1:1}`
3. `toHalfOpenFileRange()` returns `SourceRange{1:1, 1:2}`
4. the condition for `SourceLocation L` in `halfOpenRangeContains()` is `1:1 <= 
LOffset && LOffset < 1:2` which is only ever true for L == 1:1

Do I understand it right?



Comment at: clangd/refactor/actions/SwapIfBranches.cpp:62
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).

Just a typo in comment `s/brances/branches/`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

A deliberately simple syntactic transformation. Missing tests, but should work 
very reliably. To serve as an reference point for writing similar actions.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56611



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

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric, mgorny.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56611

Files:
  clangd/CMakeLists.txt
  clangd/CodeActions.cpp
  clangd/refactor/actions/SwapIfBranches.cpp
  clangd/refactor/actions/SwapIfBranches.h

Index: clangd/refactor/actions/SwapIfBranches.h
===
--- /dev/null
+++ clangd/refactor/actions/SwapIfBranches.h
@@ -0,0 +1,31 @@
+//===--- SwapIfBrances.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// A code action that swaps the 'then' and 'else' branch of the if statement.
+// Before:
+//   if (foo) { return 10; } else { continue; }
+// After:
+//   if (foo) { continue; } else { return 10; }
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_SWAPIFBRANCHES_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_SWAPIFBRANCHES_H
+
+#include "refactor/ActionProvider.h"
+
+namespace clang {
+namespace clangd {
+
+class SwapIfBranches : public ActionProvider {
+  llvm::Optional prepare(const ActionInputs ) override;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clangd/refactor/actions/SwapIfBranches.cpp
===
--- /dev/null
+++ clangd/refactor/actions/SwapIfBranches.cpp
@@ -0,0 +1,94 @@
+//===--- SwapIfBranches.cpp --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "SwapIfBranches.h"
+#include "ClangdUnit.h"
+#include "CodeActions.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+class FindIfUnderCursor : public RecursiveASTVisitor {
+public:
+  FindIfUnderCursor(ASTContext , SourceLocation CursorLoc, IfStmt *)
+  : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+ SourceRange(If->getIfLoc()));
+if (!R)
+  return true;
+if (!halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc))
+  return true;
+Result = If;
+return false;
+  }
+
+private:
+  ASTContext 
+  SourceLocation CursorLoc;
+  IfStmt *
+};
+
+llvm::Optional
+SwapIfBranches::prepare(const ActionInputs ) {
+  auto  = Inputs.AST.getASTContext();
+  auto  = Ctx.getSourceManager();
+  IfStmt *If = nullptr;
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+return llvm::None;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!llvm::dyn_cast_or_null(If->getThen()) ||
+  !llvm::dyn_cast_or_null(If->getElse()))
+return llvm::None;
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getThen()->getSourceRange());
+  if (!ThenRng)
+return llvm::None;
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+ If->getElse()->getSourceRange());
+  if (!ElseRng)
+return llvm::None;
+
+  llvm::StringRef ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  llvm::StringRef ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  tooling::Replacements R;
+  if (auto Err = R.add(tooling::Replacement(SrcMgr, ThenRng->getBegin(),
+ThenCode.size(), ElseCode))) {
+llvm::consumeError(std::move(Err));
+return llvm::None;
+  }
+  if (auto Err = R.add(tooling::Replacement(SrcMgr, ElseRng->getBegin(),
+ElseCode.size(), ThenCode))) {
+llvm::consumeError(std::move(Err));
+return llvm::None;
+  }
+  return PreparedAction("Swap if branches", std::move(R));
+}
+} // namespace clangd
+} //