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.sheph...@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 2e948c23569f6..59475b0dfd3d2 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 0000000000000..e457319dfc303 --- /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 9cd195eaf164f..b2d8cc13ed8e0 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 0000000000000..ff5d29e2e59eb --- /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 255dca3a6015a..8d19295b4d3d8 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 90b231b5796d2..686ef7f63775b 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.sheph...@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 e457319dfc303..2241b0ac37b52 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 ff5d29e2e59eb..bbb2a576af6eb 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
