Author: Tor Shepherd Date: 2024-10-03T18:34:02+02:00 New Revision: 81fcdc63594d94aa2111422e758a24eb9fc88066
URL: https://github.com/llvm/llvm-project/commit/81fcdc63594d94aa2111422e758a24eb9fc88066 DIFF: https://github.com/llvm/llvm-project/commit/81fcdc63594d94aa2111422e758a24eb9fc88066.diff LOG: [clangd] Add CodeAction to swap operands to binary operators (#78999) This MR resolves https://github.com/llvm/llvm-project/issues/78998 Added: clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp Modified: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt clang-tools-extra/clangd/unittests/CMakeLists.txt clang-tools-extra/docs/ReleaseNotes.rst llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt index 2e948c23569f68..59475b0dfd3d22 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 00000000000000..1602093e3d2893 --- /dev/null +++ b/clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp @@ -0,0 +1,216 @@ +//===--- 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 "Protocol.h" +#include "Selection.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/LLVM.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Tooling/Core/Replacement.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Casting.h" +#include "llvm/Support/FormatVariadic.h" +#include <string> +#include <utility> + +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. +bool isOpSwappable(const BinaryOperatorKind Opcode) { + switch (Opcode) { + case BinaryOperatorKind::BO_Mul: + case BinaryOperatorKind::BO_Add: + 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: + // <=> is noncommutative + case BinaryOperatorKind::BO_Cmp: + // 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 +BinaryOperatorKind swapOperator(const BinaryOperatorKind Opcode) { + switch (Opcode) { + case BinaryOperatorKind::BO_LT: + return BinaryOperatorKind::BO_GT; + + case BinaryOperatorKind::BO_GT: + return BinaryOperatorKind::BO_LT; + + case BinaryOperatorKind::BO_LE: + return BinaryOperatorKind::BO_GE; + + case BinaryOperatorKind::BO_GE: + return BinaryOperatorKind::BO_LE; + + 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 Opcode; + } +} + +/// 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 one of the operands. + // This makes sure that the selection point is in the 'scope' of the binary + // operator, not from somewhere inside a lambda for example + // (5 < [](){ ^return 1; }) + 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) { + const auto &Ctx = Inputs.AST->getASTContext(); + const auto &SrcMgr = Inputs.AST->getSourceManager(); + + const auto LHSRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(), + Op->getLHS()->getSourceRange()); + if (!LHSRng) + return error( + "Could not obtain range of the 'lhs' of the operator. Macros?"); + const auto RHSRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(), + Op->getRHS()->getSourceRange()); + if (!RHSRng) + return error( + "Could not obtain range of the 'rhs' of the operator. Macros?"); + const auto OpRng = + toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(), Op->getOperatorLoc()); + if (!OpRng) + return error("Could not obtain range of the operator itself. Macros?"); + + const auto LHSCode = toSourceCode(SrcMgr, *LHSRng); + const auto RHSCode = toSourceCode(SrcMgr, *RHSRng); + const 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); + const auto SwappedOperator = swapOperator(Op->getOpcode()); + 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 4fa9f18407ae9e..dffdcd5d014ca9 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -137,6 +137,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 00000000000000..e157bbefbdaaf7 --- /dev/null +++ b/clang-tools-extra/clangd/unittests/tweaks/SwapBinaryOperandsTests.cpp @@ -0,0 +1,92 @@ +//===-- 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;"); + EXPECT_EQ(apply("int f(); int x = 3; bool c = x >=^ f();"), + "int f(); int x = 3; bool c = f() <= x;"); + EXPECT_EQ(apply(R"cpp( + int f(); + #define F f + int x = 3; bool c = x >=^ F(); + )cpp"), + R"cpp( + int f(); + #define F f + int x = 3; bool c = F() <= x; + )cpp"); + EXPECT_EQ(apply(R"cpp( + int f(); + #define F f() + int x = 3; bool c = x >=^ F; + )cpp"), + R"cpp( + int f(); + #define F f() + int x = 3; bool c = F <= x; + )cpp"); + EXPECT_EQ(apply(R"cpp( + int f(bool); + #define F(v) f(v) + int x = 0; + bool c = F(x^ < 5); + )cpp"), + R"cpp( + int f(bool); + #define F(v) f(v) + int x = 0; + bool c = F(5 > x); + )cpp"); + ExtraArgs = {"-std=c++20"}; + Context = CodeContext::File; + EXPECT_UNAVAILABLE(R"cpp( + namespace std { + struct strong_ordering { + int val; + static const strong_ordering less; + static const strong_ordering equivalent; + static const strong_ordering equal; + static const strong_ordering greater; + }; + inline constexpr strong_ordering strong_ordering::less {-1}; + inline constexpr strong_ordering strong_ordering::equivalent {0}; + inline constexpr strong_ordering strong_ordering::equal {0}; + inline constexpr strong_ordering strong_ordering::greater {1}; + }; + #define F(v) v + int x = 0; + auto c = F(5^ <=> x); + )cpp"); +} + +} // namespace +} // namespace clangd +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e4cef50dd26075..97300f12eab628 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -74,6 +74,8 @@ Code completion Code actions ^^^^^^^^^^^^ +- Added `Swap operands` tweak for certain binary operators. + Signature help ^^^^^^^^^^^^^^ 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 255dca3a6015a5..8d19295b4d3d84 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 b46bac97b41fa9..7deefe9dc06137 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 @@ -150,6 +150,7 @@ unittest("ClangdTests") { "tweaks/ScopifyEnumTests.cpp", "tweaks/ShowSelectionTreeTests.cpp", "tweaks/SpecialMembersTests.cpp", + "tweaks/SwapBinaryOperandsTests.cpp", "tweaks/SwapIfBranchesTests.cpp", "tweaks/TweakTesting.cpp", "tweaks/TweakTests.cpp", _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
