[clangd] Add CodeAction to swap operands to binary operators #78999

torshepherd · 2024-01-22T16:04:29Z

This MR resolves #78998

github-actions · 2024-01-22T16:04:46Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-01-22T16:05:16Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Tor Shepherd (torshepherd)

Changes

This MR resolves #78998

Full diff: https://github.com/llvm/llvm-project/pull/78999.diff

6 Files Affected:

(modified) clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt (+1)
(added) clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp (+210)
(modified) clang-tools-extra/clangd/unittests/CMakeLists.txt (+1)
(added) clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp (+38)
(modified) llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn (+1)
(modified) llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn (+1)

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..2241b0ac37b52ab
--- /dev/null
+++ b/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp
@@ -0,0 +1,210 @@
+//===--- 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 {0}",
+                         Op ? Op->getOpcodeStr() : "binary operator");
+  }
+  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;
+    }
+  }
+  return Op != nullptr;
+}
+
+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..bbb2a576af6eb9d
--- /dev/null
+++ b/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp
@@ -0,0 +1,38 @@
+//===-- 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("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
+} // 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",

clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp

torshepherd · 2024-05-04T14:31:10Z

Ping

5chmidti

Could you add a release note for this as well?

please bring the branch up to date with main

clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp

clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp

clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp

torshepherd · 2024-09-23T23:40:01Z

Could you add a release note for this as well?
* please bring the branch up to date with main

Where do I add clangd release notes?

Edit: Ok I tried something, lmk if this looks good or needs more work

torshepherd · 2024-10-01T13:11:40Z

@5chmidti this should be ready for merge

5chmidti · 2024-10-02T17:13:11Z

@5chmidti this should be ready for merge

Yep.

Though, please check out: https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it/74223
TLDR: LLVM wants commit emails that can be reached out to. I have created a new email address for this to separate that from my private one. You'd need to add the one you would like to commit as to your emails and disable keeping emails private: https://github.com/settings/emails. Sadly, that is an all or nothing toggle.
(Please mention which one I should choose in the UI. That can be deduced by GitHub by changing the local git config for the LLVM repo to use that email)

torshepherd · 2024-10-02T17:27:30Z

I'll try to rectify this; for now you can add

tor [dot] aksel [dot] shepherd [at] gmail [dot] com

llvmbot added clang-tools-extra clangd labels Jan 22, 2024

torshepherd commented Jan 26, 2024

View reviewed changes

clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp Outdated Show resolved Hide resolved

torshepherd force-pushed the clangd-swap-binary-operands branch from 0870447 to 6abe2b5 Compare February 11, 2024 19:46

5chmidti reviewed Sep 13, 2024

View reviewed changes

[clangd] Swap binary operands

718e26d

torshepherd force-pushed the clangd-swap-binary-operands branch from 6abe2b5 to 718e26d Compare September 23, 2024 23:27

Schmidti comments

328e57a

Trying my hand at rel notes

b912072

5chmidti merged commit 81fcdc6 into llvm:main Oct 3, 2024
7 checks passed

torshepherd deleted the clangd-swap-binary-operands branch October 3, 2024 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clangd] Add CodeAction to swap operands to binary operators #78999

[clangd] Add CodeAction to swap operands to binary operators #78999

Uh oh!

torshepherd commented Jan 22, 2024

Uh oh!

github-actions bot commented Jan 22, 2024

Uh oh!

llvmbot commented Jan 22, 2024 •

edited

Loading

Uh oh!

Uh oh!

torshepherd commented May 4, 2024

Uh oh!

5chmidti left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

torshepherd commented Sep 23, 2024 •

edited

Loading

Uh oh!

torshepherd commented Oct 1, 2024

Uh oh!

5chmidti commented Oct 2, 2024

Uh oh!

torshepherd commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

[clangd] Add CodeAction to swap operands to binary operators #78999

[clangd] Add CodeAction to swap operands to binary operators #78999

Uh oh!

Conversation

torshepherd commented Jan 22, 2024

Uh oh!

github-actions bot commented Jan 22, 2024

Uh oh!

llvmbot commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

torshepherd commented May 4, 2024

Uh oh!

5chmidti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

torshepherd commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

torshepherd commented Oct 1, 2024

Uh oh!

5chmidti commented Oct 2, 2024

Uh oh!

torshepherd commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jan 22, 2024 •

edited

Loading

5chmidti left a comment •

edited

Loading

torshepherd commented Sep 23, 2024 •

edited

Loading