[llvm] [clang-tools-extra] [clangd] Add CodeAction to swap operands to binary operators (PR #78999)

Tor Shepherd via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 24 10:20:09 PST 2024


https://github.com/torshepherd updated https://github.com/llvm/llvm-project/pull/78999

>From c22c4fc969af0860b1fb2c371d31c2757da70e01 Mon Sep 17 00:00:00 2001
From: Tor Shepherd <tor.aksel.shepherd at gmail.com>
Date: Sun, 21 Jan 2024 17:53:31 -0500
Subject: [PATCH 1/2] swap binary

---
 .../clangd/refactor/tweaks/CMakeLists.txt     |   1 +
 .../refactor/tweaks/SwapBinaryOperands.cpp    | 214 ++++++++++++++++++
 .../clangd/unittests/CMakeLists.txt           |   1 +
 .../tweaks/SwapBinaryOperandsTests.cpp        |  29 +++
 .../clangd/refactor/tweaks/BUILD.gn           |   1 +
 .../clangd/unittests/BUILD.gn                 |   1 +
 6 files changed, 247 insertions(+)
 create mode 100644 clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp
 create mode 100644 clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp

diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
index 2e948c23569f686..59475b0dfd3d222 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -29,6 +29,7 @@ add_clang_library(clangDaemonTweaks OBJECT
   RemoveUsingNamespace.cpp
   ScopifyEnum.cpp
   SpecialMembers.cpp
+  SwapBinaryOperands.cpp
   SwapIfBranches.cpp
 
   LINK_LIBS
diff --git a/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp b/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp
new file mode 100644
index 000000000000000..e457319dfc30370
--- /dev/null
+++ b/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp
@@ -0,0 +1,214 @@
+//===--- SwapBinaryOperands.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 "ParsedAST.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "support/Logger.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/OperationKinds.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+/// Check whether it makes logical sense to swap operands to an operator.
+/// Assignment or member access operators are rarely swappable
+/// while keeping the meaning intact, whereas comparison operators, mathematical
+/// operators, etc. are often desired to be swappable for readability, avoiding
+/// bugs by assigning to nullptr when comparison was desired, etc.
+auto isOpSwappable(const BinaryOperatorKind Opcode) -> bool {
+  switch (Opcode) {
+  case BinaryOperatorKind::BO_Mul:
+  case BinaryOperatorKind::BO_Add:
+  case BinaryOperatorKind::BO_Cmp:
+  case BinaryOperatorKind::BO_LT:
+  case BinaryOperatorKind::BO_GT:
+  case BinaryOperatorKind::BO_LE:
+  case BinaryOperatorKind::BO_GE:
+  case BinaryOperatorKind::BO_EQ:
+  case BinaryOperatorKind::BO_NE:
+  case BinaryOperatorKind::BO_And:
+  case BinaryOperatorKind::BO_Xor:
+  case BinaryOperatorKind::BO_Or:
+  case BinaryOperatorKind::BO_LAnd:
+  case BinaryOperatorKind::BO_LOr:
+  case BinaryOperatorKind::BO_Comma:
+    return true;
+  // Noncommutative operators:
+  case BinaryOperatorKind::BO_Div:
+  case BinaryOperatorKind::BO_Sub:
+  case BinaryOperatorKind::BO_Shl:
+  case BinaryOperatorKind::BO_Shr:
+  case BinaryOperatorKind::BO_Rem:
+  // Member access:
+  case BinaryOperatorKind::BO_PtrMemD:
+  case BinaryOperatorKind::BO_PtrMemI:
+  // Assignment:
+  case BinaryOperatorKind::BO_Assign:
+  case BinaryOperatorKind::BO_MulAssign:
+  case BinaryOperatorKind::BO_DivAssign:
+  case BinaryOperatorKind::BO_RemAssign:
+  case BinaryOperatorKind::BO_AddAssign:
+  case BinaryOperatorKind::BO_SubAssign:
+  case BinaryOperatorKind::BO_ShlAssign:
+  case BinaryOperatorKind::BO_ShrAssign:
+  case BinaryOperatorKind::BO_AndAssign:
+  case BinaryOperatorKind::BO_XorAssign:
+  case BinaryOperatorKind::BO_OrAssign:
+    return false;
+  }
+  return false;
+}
+
+/// Some operators are asymmetric and need to be flipped when swapping their
+/// operands
+/// @param[out] Opcode the opcode to potentially swap
+/// If the opcode does not need to be swapped or is not swappable, does nothing
+void swapOperator(BinaryOperatorKind &Opcode) {
+  switch (Opcode) {
+  case BinaryOperatorKind::BO_LT:
+    Opcode = BinaryOperatorKind::BO_GT;
+    return;
+  case BinaryOperatorKind::BO_GT:
+    Opcode = BinaryOperatorKind::BO_LT;
+    return;
+  case BinaryOperatorKind::BO_LE:
+    Opcode = BinaryOperatorKind::BO_GE;
+    return;
+  case BinaryOperatorKind::BO_GE:
+    Opcode = BinaryOperatorKind::BO_LE;
+    return;
+  case BinaryOperatorKind::BO_Mul:
+  case BinaryOperatorKind::BO_Add:
+  case BinaryOperatorKind::BO_Cmp:
+  case BinaryOperatorKind::BO_EQ:
+  case BinaryOperatorKind::BO_NE:
+  case BinaryOperatorKind::BO_And:
+  case BinaryOperatorKind::BO_Xor:
+  case BinaryOperatorKind::BO_Or:
+  case BinaryOperatorKind::BO_LAnd:
+  case BinaryOperatorKind::BO_LOr:
+  case BinaryOperatorKind::BO_Comma:
+  case BinaryOperatorKind::BO_Div:
+  case BinaryOperatorKind::BO_Sub:
+  case BinaryOperatorKind::BO_Shl:
+  case BinaryOperatorKind::BO_Shr:
+  case BinaryOperatorKind::BO_Rem:
+  case BinaryOperatorKind::BO_PtrMemD:
+  case BinaryOperatorKind::BO_PtrMemI:
+  case BinaryOperatorKind::BO_Assign:
+  case BinaryOperatorKind::BO_MulAssign:
+  case BinaryOperatorKind::BO_DivAssign:
+  case BinaryOperatorKind::BO_RemAssign:
+  case BinaryOperatorKind::BO_AddAssign:
+  case BinaryOperatorKind::BO_SubAssign:
+  case BinaryOperatorKind::BO_ShlAssign:
+  case BinaryOperatorKind::BO_ShrAssign:
+  case BinaryOperatorKind::BO_AndAssign:
+  case BinaryOperatorKind::BO_XorAssign:
+  case BinaryOperatorKind::BO_OrAssign:
+    return;
+  }
+}
+
+/// Swaps the operands to a binary operator
+/// Before:
+///   x != nullptr
+///   ^    ^^^^^^^
+/// After:
+///   nullptr != x
+class SwapBinaryOperands : public Tweak {
+public:
+  const char *id() const final;
+
+  bool prepare(const Selection &Inputs) override;
+  Expected<Effect> apply(const Selection &Inputs) override;
+  std::string title() const override {
+    return llvm::formatv("Swap operands to binary operator {0}",
+                         Op ? Op->getOpcodeStr() : "");
+  }
+  llvm::StringLiteral kind() const override {
+    return CodeAction::REFACTOR_KIND;
+  }
+  bool hidden() const override { return false; }
+
+private:
+  const BinaryOperator *Op;
+};
+
+REGISTER_TWEAK(SwapBinaryOperands)
+
+bool SwapBinaryOperands::prepare(const Selection &Inputs) {
+  for (const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
+       N && !Op; N = N->Parent) {
+    // Stop once we hit a block, e.g. a lambda in the if condition.
+    if (llvm::isa_and_nonnull<CompoundStmt>(N->ASTNode.get<Stmt>()))
+      return false;
+    Op = dyn_cast_or_null<BinaryOperator>(N->ASTNode.get<Stmt>());
+    // If we hit upon a nonswappable binary operator, ignore and keep going
+    if (Op && !isOpSwappable(Op->getOpcode())) {
+      Op = nullptr;
+    }
+  }
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  // FIXME: remove this
+  return Op && isa_and_nonnull<CompoundStmt>(Op->getLHS()) &&
+         isa_and_nonnull<CompoundStmt>(Op->getRHS());
+}
+
+Expected<Tweak::Effect> SwapBinaryOperands::apply(const Selection &Inputs) {
+  auto &Ctx = Inputs.AST->getASTContext();
+  auto &SrcMgr = Inputs.AST->getSourceManager();
+
+  auto LHSRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+                                    Op->getLHS()->getSourceRange());
+  if (!LHSRng)
+    return error(
+        "Could not obtain range of the 'lhs' of the operator. Macros?");
+  auto RHSRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+                                    Op->getRHS()->getSourceRange());
+  if (!RHSRng)
+    return error(
+        "Could not obtain range of the 'rhs' of the operator. Macros?");
+  auto OpRng =
+      toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(), Op->getOperatorLoc());
+  if (!OpRng)
+    return error("Could not obtain range of the operator itself. Macros?");
+
+  auto LHSCode = toSourceCode(SrcMgr, *LHSRng);
+  auto RHSCode = toSourceCode(SrcMgr, *RHSRng);
+  auto OperatorCode = toSourceCode(SrcMgr, *OpRng);
+
+  tooling::Replacements Result;
+  if (auto Err = Result.add(tooling::Replacement(
+          Ctx.getSourceManager(), LHSRng->getBegin(), LHSCode.size(), RHSCode)))
+    return std::move(Err);
+  if (auto Err = Result.add(tooling::Replacement(
+          Ctx.getSourceManager(), RHSRng->getBegin(), RHSCode.size(), LHSCode)))
+    return std::move(Err);
+  auto SwappedOperator = Op->getOpcode();
+  swapOperator(SwappedOperator);
+  if (auto Err = Result.add(tooling::Replacement(
+          Ctx.getSourceManager(), OpRng->getBegin(), OperatorCode.size(),
+          Op->getOpcodeStr(SwappedOperator))))
+    return std::move(Err);
+  return Effect::mainFileEdit(SrcMgr, std::move(Result));
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt
index 9cd195eaf164fb6..b2d8cc13ed8e07c 100644
--- a/clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -134,6 +134,7 @@ add_unittest(ClangdUnitTests ClangdTests
   tweaks/ScopifyEnumTests.cpp
   tweaks/ShowSelectionTreeTests.cpp
   tweaks/SpecialMembersTests.cpp
+  tweaks/SwapBinaryOperandsTests.cpp
   tweaks/SwapIfBranchesTests.cpp
   tweaks/TweakTesting.cpp
   tweaks/TweakTests.cpp
diff --git a/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp
new file mode 100644
index 000000000000000..ff5d29e2e59eb93
--- /dev/null
+++ b/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp
@@ -0,0 +1,29 @@
+//===-- SwapBinaryOperandsTests.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 "TweakTesting.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TWEAK_TEST(SwapBinaryOperands);
+
+TEST_F(SwapBinaryOperandsTest, Test) {
+  Context = Function;
+  EXPECT_EQ(apply("^p == nullptr"), "nullptr == p");
+  EXPECT_EQ(apply("^p == nullptr"), "nullptr == p");
+  EXPECT_EQ(apply("^x >= 5"), "5 <= x");
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
diff --git a/llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn b/llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
index 255dca3a6015a54..8d19295b4d3d84a 100644
--- a/llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -35,6 +35,7 @@ source_set("tweaks") {
     "RemoveUsingNamespace.cpp",
     "ScopifyEnum.cpp",
     "SpecialMembers.cpp",
+    "SwapBinaryOperands.cpp",
     "SwapIfBranches.cpp",
   ]
 }
diff --git a/llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn b/llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
index 90b231b5796d278..686ef7f63775bcf 100644
--- a/llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -149,6 +149,7 @@ unittest("ClangdTests") {
     "tweaks/ScopifyEnumTests.cpp",
     "tweaks/ShowSelectionTreeTests.cpp",
     "tweaks/SpecialMembersTests.cpp",
+    "tweaks/SwapBinaryOperandsTests.cpp",
     "tweaks/SwapIfBranchesTests.cpp",
     "tweaks/TweakTesting.cpp",
     "tweaks/TweakTests.cpp",

>From c1dc72852449655bdd7debba2aa62df47c155dbf Mon Sep 17 00:00:00 2001
From: Tor Shepherd <tor.aksel.shepherd at gmail.com>
Date: Sun, 21 Jan 2024 22:33:53 -0500
Subject: [PATCH 2/2] works!

---
 .../clangd/refactor/tweaks/SwapBinaryOperands.cpp | 10 +++-------
 .../unittests/tweaks/SwapBinaryOperandsTests.cpp  | 15 ++++++++++++---
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp b/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp
index e457319dfc30370..2241b0ac37b52ab 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp
@@ -137,8 +137,8 @@ class SwapBinaryOperands : public Tweak {
   bool prepare(const Selection &Inputs) override;
   Expected<Effect> apply(const Selection &Inputs) override;
   std::string title() const override {
-    return llvm::formatv("Swap operands to binary operator {0}",
-                         Op ? Op->getOpcodeStr() : "");
+    return llvm::formatv("Swap operands to {0}",
+                         Op ? Op->getOpcodeStr() : "binary operator");
   }
   llvm::StringLiteral kind() const override {
     return CodeAction::REFACTOR_KIND;
@@ -163,11 +163,7 @@ bool SwapBinaryOperands::prepare(const Selection &Inputs) {
       Op = nullptr;
     }
   }
-  // avoid dealing with single-statement brances, they require careful handling
-  // to avoid changing semantics of the code (i.e. dangling else).
-  // FIXME: remove this
-  return Op && isa_and_nonnull<CompoundStmt>(Op->getLHS()) &&
-         isa_and_nonnull<CompoundStmt>(Op->getRHS());
+  return Op != nullptr;
 }
 
 Expected<Tweak::Effect> SwapBinaryOperands::apply(const Selection &Inputs) {
diff --git a/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp
index ff5d29e2e59eb93..bbb2a576af6eb9d 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp
@@ -19,9 +19,18 @@ TWEAK_TEST(SwapBinaryOperands);
 
 TEST_F(SwapBinaryOperandsTest, Test) {
   Context = Function;
-  EXPECT_EQ(apply("^p == nullptr"), "nullptr == p");
-  EXPECT_EQ(apply("^p == nullptr"), "nullptr == p");
-  EXPECT_EQ(apply("^x >= 5"), "5 <= x");
+  EXPECT_EQ(apply("int *p = nullptr; bool c = ^p == nullptr;"),
+            "int *p = nullptr; bool c = nullptr == p;");
+  EXPECT_EQ(apply("int *p = nullptr; bool c = p ^== nullptr;"),
+            "int *p = nullptr; bool c = nullptr == p;");
+  EXPECT_EQ(apply("int x = 3; bool c = ^x >= 5;"),
+            "int x = 3; bool c = 5 <= x;");
+  EXPECT_EQ(apply("int x = 3; bool c = x >^= 5;"),
+            "int x = 3; bool c = 5 <= x;");
+  EXPECT_EQ(apply("int x = 3; bool c = x >=^ 5;"),
+            "int x = 3; bool c = 5 <= x;");
+  EXPECT_EQ(apply("int x = 3; bool c = x >=^ 5;"),
+            "int x = 3; bool c = 5 <= x;");
 }
 
 } // namespace



More information about the cfe-commits mailing list