Skip to content

Commit 81fcdc6

Browse files
torshepherdtorshepherd
authored
[clangd] Add CodeAction to swap operands to binary operators (llvm#78999)
This MR resolves llvm#78998
1 parent 06c8210 commit 81fcdc6

File tree

7 files changed

+314
-0
lines changed

7 files changed

+314
-0
lines changed

‎clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ add_clang_library(clangDaemonTweaks OBJECT
2929
RemoveUsingNamespace.cpp
3030
ScopifyEnum.cpp
3131
SpecialMembers.cpp
32+
SwapBinaryOperands.cpp
3233
SwapIfBranches.cpp
3334

3435
LINK_LIBS

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

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
//===--- SwapBinaryOperands.cpp ----------------------------------*- C++-*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
#include "ParsedAST.h"
9+
#include "Protocol.h"
10+
#include "Selection.h"
11+
#include "SourceCode.h"
12+
#include "refactor/Tweak.h"
13+
#include "support/Logger.h"
14+
#include "clang/AST/ASTContext.h"
15+
#include "clang/AST/Expr.h"
16+
#include "clang/AST/OperationKinds.h"
17+
#include "clang/AST/Stmt.h"
18+
#include "clang/Basic/LLVM.h"
19+
#include "clang/Basic/SourceLocation.h"
20+
#include "clang/Tooling/Core/Replacement.h"
21+
#include "llvm/ADT/StringRef.h"
22+
#include "llvm/Support/Casting.h"
23+
#include "llvm/Support/FormatVariadic.h"
24+
#include <string>
25+
#include <utility>
26+
27+
namespace clang {
28+
namespace clangd {
29+
namespace {
30+
/// Check whether it makes logical sense to swap operands to an operator.
31+
/// Assignment or member access operators are rarely swappable
32+
/// while keeping the meaning intact, whereas comparison operators, mathematical
33+
/// operators, etc. are often desired to be swappable for readability, avoiding
34+
/// bugs by assigning to nullptr when comparison was desired, etc.
35+
bool isOpSwappable(const BinaryOperatorKind Opcode) {
36+
switch (Opcode) {
37+
case BinaryOperatorKind::BO_Mul:
38+
case BinaryOperatorKind::BO_Add:
39+
case BinaryOperatorKind::BO_LT:
40+
case BinaryOperatorKind::BO_GT:
41+
case BinaryOperatorKind::BO_LE:
42+
case BinaryOperatorKind::BO_GE:
43+
case BinaryOperatorKind::BO_EQ:
44+
case BinaryOperatorKind::BO_NE:
45+
case BinaryOperatorKind::BO_And:
46+
case BinaryOperatorKind::BO_Xor:
47+
case BinaryOperatorKind::BO_Or:
48+
case BinaryOperatorKind::BO_LAnd:
49+
case BinaryOperatorKind::BO_LOr:
50+
case BinaryOperatorKind::BO_Comma:
51+
return true;
52+
// Noncommutative operators:
53+
case BinaryOperatorKind::BO_Div:
54+
case BinaryOperatorKind::BO_Sub:
55+
case BinaryOperatorKind::BO_Shl:
56+
case BinaryOperatorKind::BO_Shr:
57+
case BinaryOperatorKind::BO_Rem:
58+
// <=> is noncommutative
59+
case BinaryOperatorKind::BO_Cmp:
60+
// Member access:
61+
case BinaryOperatorKind::BO_PtrMemD:
62+
case BinaryOperatorKind::BO_PtrMemI:
63+
// Assignment:
64+
case BinaryOperatorKind::BO_Assign:
65+
case BinaryOperatorKind::BO_MulAssign:
66+
case BinaryOperatorKind::BO_DivAssign:
67+
case BinaryOperatorKind::BO_RemAssign:
68+
case BinaryOperatorKind::BO_AddAssign:
69+
case BinaryOperatorKind::BO_SubAssign:
70+
case BinaryOperatorKind::BO_ShlAssign:
71+
case BinaryOperatorKind::BO_ShrAssign:
72+
case BinaryOperatorKind::BO_AndAssign:
73+
case BinaryOperatorKind::BO_XorAssign:
74+
case BinaryOperatorKind::BO_OrAssign:
75+
return false;
76+
}
77+
return false;
78+
}
79+
80+
/// Some operators are asymmetric and need to be flipped when swapping their
81+
/// operands
82+
/// @param[out] Opcode the opcode to potentially swap
83+
/// If the opcode does not need to be swapped or is not swappable, does nothing
84+
BinaryOperatorKind swapOperator(const BinaryOperatorKind Opcode) {
85+
switch (Opcode) {
86+
case BinaryOperatorKind::BO_LT:
87+
return BinaryOperatorKind::BO_GT;
88+
89+
case BinaryOperatorKind::BO_GT:
90+
return BinaryOperatorKind::BO_LT;
91+
92+
case BinaryOperatorKind::BO_LE:
93+
return BinaryOperatorKind::BO_GE;
94+
95+
case BinaryOperatorKind::BO_GE:
96+
return BinaryOperatorKind::BO_LE;
97+
98+
case BinaryOperatorKind::BO_Mul:
99+
case BinaryOperatorKind::BO_Add:
100+
case BinaryOperatorKind::BO_Cmp:
101+
case BinaryOperatorKind::BO_EQ:
102+
case BinaryOperatorKind::BO_NE:
103+
case BinaryOperatorKind::BO_And:
104+
case BinaryOperatorKind::BO_Xor:
105+
case BinaryOperatorKind::BO_Or:
106+
case BinaryOperatorKind::BO_LAnd:
107+
case BinaryOperatorKind::BO_LOr:
108+
case BinaryOperatorKind::BO_Comma:
109+
case BinaryOperatorKind::BO_Div:
110+
case BinaryOperatorKind::BO_Sub:
111+
case BinaryOperatorKind::BO_Shl:
112+
case BinaryOperatorKind::BO_Shr:
113+
case BinaryOperatorKind::BO_Rem:
114+
case BinaryOperatorKind::BO_PtrMemD:
115+
case BinaryOperatorKind::BO_PtrMemI:
116+
case BinaryOperatorKind::BO_Assign:
117+
case BinaryOperatorKind::BO_MulAssign:
118+
case BinaryOperatorKind::BO_DivAssign:
119+
case BinaryOperatorKind::BO_RemAssign:
120+
case BinaryOperatorKind::BO_AddAssign:
121+
case BinaryOperatorKind::BO_SubAssign:
122+
case BinaryOperatorKind::BO_ShlAssign:
123+
case BinaryOperatorKind::BO_ShrAssign:
124+
case BinaryOperatorKind::BO_AndAssign:
125+
case BinaryOperatorKind::BO_XorAssign:
126+
case BinaryOperatorKind::BO_OrAssign:
127+
return Opcode;
128+
}
129+
}
130+
131+
/// Swaps the operands to a binary operator
132+
/// Before:
133+
/// x != nullptr
134+
/// ^ ^^^^^^^
135+
/// After:
136+
/// nullptr != x
137+
class SwapBinaryOperands : public Tweak {
138+
public:
139+
const char *id() const final;
140+
141+
bool prepare(const Selection &Inputs) override;
142+
Expected<Effect> apply(const Selection &Inputs) override;
143+
std::string title() const override {
144+
return llvm::formatv("Swap operands to {0}",
145+
Op ? Op->getOpcodeStr() : "binary operator");
146+
}
147+
llvm::StringLiteral kind() const override {
148+
return CodeAction::REFACTOR_KIND;
149+
}
150+
bool hidden() const override { return false; }
151+
152+
private:
153+
const BinaryOperator *Op;
154+
};
155+
156+
REGISTER_TWEAK(SwapBinaryOperands)
157+
158+
bool SwapBinaryOperands::prepare(const Selection &Inputs) {
159+
for (const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
160+
N && !Op; N = N->Parent) {
161+
// Stop once we hit a block, e.g. a lambda in one of the operands.
162+
// This makes sure that the selection point is in the 'scope' of the binary
163+
// operator, not from somewhere inside a lambda for example
164+
// (5 < [](){ ^return 1; })
165+
if (llvm::isa_and_nonnull<CompoundStmt>(N->ASTNode.get<Stmt>()))
166+
return false;
167+
Op = dyn_cast_or_null<BinaryOperator>(N->ASTNode.get<Stmt>());
168+
// If we hit upon a nonswappable binary operator, ignore and keep going
169+
if (Op && !isOpSwappable(Op->getOpcode())) {
170+
Op = nullptr;
171+
}
172+
}
173+
return Op != nullptr;
174+
}
175+
176+
Expected<Tweak::Effect> SwapBinaryOperands::apply(const Selection &Inputs) {
177+
const auto &Ctx = Inputs.AST->getASTContext();
178+
const auto &SrcMgr = Inputs.AST->getSourceManager();
179+
180+
const auto LHSRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
181+
Op->getLHS()->getSourceRange());
182+
if (!LHSRng)
183+
return error(
184+
"Could not obtain range of the 'lhs' of the operator. Macros?");
185+
const auto RHSRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
186+
Op->getRHS()->getSourceRange());
187+
if (!RHSRng)
188+
return error(
189+
"Could not obtain range of the 'rhs' of the operator. Macros?");
190+
const auto OpRng =
191+
toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(), Op->getOperatorLoc());
192+
if (!OpRng)
193+
return error("Could not obtain range of the operator itself. Macros?");
194+
195+
const auto LHSCode = toSourceCode(SrcMgr, *LHSRng);
196+
const auto RHSCode = toSourceCode(SrcMgr, *RHSRng);
197+
const auto OperatorCode = toSourceCode(SrcMgr, *OpRng);
198+
199+
tooling::Replacements Result;
200+
if (auto Err = Result.add(tooling::Replacement(
201+
Ctx.getSourceManager(), LHSRng->getBegin(), LHSCode.size(), RHSCode)))
202+
return std::move(Err);
203+
if (auto Err = Result.add(tooling::Replacement(
204+
Ctx.getSourceManager(), RHSRng->getBegin(), RHSCode.size(), LHSCode)))
205+
return std::move(Err);
206+
const auto SwappedOperator = swapOperator(Op->getOpcode());
207+
if (auto Err = Result.add(tooling::Replacement(
208+
Ctx.getSourceManager(), OpRng->getBegin(), OperatorCode.size(),
209+
Op->getOpcodeStr(SwappedOperator))))
210+
return std::move(Err);
211+
return Effect::mainFileEdit(SrcMgr, std::move(Result));
212+
}
213+
214+
} // namespace
215+
} // namespace clangd
216+
} // namespace clang

‎clang-tools-extra/clangd/unittests/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ add_unittest(ClangdUnitTests ClangdTests
137137
tweaks/ScopifyEnumTests.cpp
138138
tweaks/ShowSelectionTreeTests.cpp
139139
tweaks/SpecialMembersTests.cpp
140+
tweaks/SwapBinaryOperandsTests.cpp
140141
tweaks/SwapIfBranchesTests.cpp
141142
tweaks/TweakTesting.cpp
142143
tweaks/TweakTests.cpp

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

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
//===-- SwapBinaryOperandsTests.cpp -----------------------------*- C++ -*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "TweakTesting.h"
10+
#include "gmock/gmock-matchers.h"
11+
#include "gmock/gmock.h"
12+
#include "gtest/gtest.h"
13+
14+
namespace clang {
15+
namespace clangd {
16+
namespace {
17+
18+
TWEAK_TEST(SwapBinaryOperands);
19+
20+
TEST_F(SwapBinaryOperandsTest, Test) {
21+
Context = Function;
22+
EXPECT_EQ(apply("int *p = nullptr; bool c = ^p == nullptr;"),
23+
"int *p = nullptr; bool c = nullptr == p;");
24+
EXPECT_EQ(apply("int *p = nullptr; bool c = p ^== nullptr;"),
25+
"int *p = nullptr; bool c = nullptr == p;");
26+
EXPECT_EQ(apply("int x = 3; bool c = ^x >= 5;"),
27+
"int x = 3; bool c = 5 <= x;");
28+
EXPECT_EQ(apply("int x = 3; bool c = x >^= 5;"),
29+
"int x = 3; bool c = 5 <= x;");
30+
EXPECT_EQ(apply("int x = 3; bool c = x >=^ 5;"),
31+
"int x = 3; bool c = 5 <= x;");
32+
EXPECT_EQ(apply("int x = 3; bool c = x >=^ 5;"),
33+
"int x = 3; bool c = 5 <= x;");
34+
EXPECT_EQ(apply("int f(); int x = 3; bool c = x >=^ f();"),
35+
"int f(); int x = 3; bool c = f() <= x;");
36+
EXPECT_EQ(apply(R"cpp(
37+
int f();
38+
#define F f
39+
int x = 3; bool c = x >=^ F();
40+
)cpp"),
41+
R"cpp(
42+
int f();
43+
#define F f
44+
int x = 3; bool c = F() <= x;
45+
)cpp");
46+
EXPECT_EQ(apply(R"cpp(
47+
int f();
48+
#define F f()
49+
int x = 3; bool c = x >=^ F;
50+
)cpp"),
51+
R"cpp(
52+
int f();
53+
#define F f()
54+
int x = 3; bool c = F <= x;
55+
)cpp");
56+
EXPECT_EQ(apply(R"cpp(
57+
int f(bool);
58+
#define F(v) f(v)
59+
int x = 0;
60+
bool c = F(x^ < 5);
61+
)cpp"),
62+
R"cpp(
63+
int f(bool);
64+
#define F(v) f(v)
65+
int x = 0;
66+
bool c = F(5 > x);
67+
)cpp");
68+
ExtraArgs = {"-std=c++20"};
69+
Context = CodeContext::File;
70+
EXPECT_UNAVAILABLE(R"cpp(
71+
namespace std {
72+
struct strong_ordering {
73+
int val;
74+
static const strong_ordering less;
75+
static const strong_ordering equivalent;
76+
static const strong_ordering equal;
77+
static const strong_ordering greater;
78+
};
79+
inline constexpr strong_ordering strong_ordering::less {-1};
80+
inline constexpr strong_ordering strong_ordering::equivalent {0};
81+
inline constexpr strong_ordering strong_ordering::equal {0};
82+
inline constexpr strong_ordering strong_ordering::greater {1};
83+
};
84+
#define F(v) v
85+
int x = 0;
86+
auto c = F(5^ <=> x);
87+
)cpp");
88+
}
89+
90+
} // namespace
91+
} // namespace clangd
92+
} // namespace clang

‎clang-tools-extra/docs/ReleaseNotes.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ Code completion
7474
Code actions
7575
^^^^^^^^^^^^
7676

77+
- Added `Swap operands` tweak for certain binary operators.
78+
7779
Signature help
7880
^^^^^^^^^^^^^^
7981

‎llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ source_set("tweaks") {
3535
"RemoveUsingNamespace.cpp",
3636
"ScopifyEnum.cpp",
3737
"SpecialMembers.cpp",
38+
"SwapBinaryOperands.cpp",
3839
"SwapIfBranches.cpp",
3940
]
4041
}

‎llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ unittest("ClangdTests") {
150150
"tweaks/ScopifyEnumTests.cpp",
151151
"tweaks/ShowSelectionTreeTests.cpp",
152152
"tweaks/SpecialMembersTests.cpp",
153+
"tweaks/SwapBinaryOperandsTests.cpp",
153154
"tweaks/SwapIfBranchesTests.cpp",
154155
"tweaks/TweakTesting.cpp",
155156
"tweaks/TweakTests.cpp",

0 commit comments

Comments
 (0)