Skip to content

Commit e8fb312

Browse files
torshepherdtorshepherd
committed
[clangd] Swap binary operands
1 parent 20ae619 commit e8fb312

File tree

6 files changed

+252
-0
lines changed

6 files changed

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

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ add_unittest(ClangdUnitTests ClangdTests
134134
tweaks/ScopifyEnumTests.cpp
135135
tweaks/ShowSelectionTreeTests.cpp
136136
tweaks/SpecialMembersTests.cpp
137+
tweaks/SwapBinaryOperandsTests.cpp
137138
tweaks/SwapIfBranchesTests.cpp
138139
tweaks/TweakTesting.cpp
139140
tweaks/TweakTests.cpp

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
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+
}
35+
36+
} // namespace
37+
} // namespace clangd
38+
} // namespace clang

‎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
@@ -149,6 +149,7 @@ unittest("ClangdTests") {
149149
"tweaks/ScopifyEnumTests.cpp",
150150
"tweaks/ShowSelectionTreeTests.cpp",
151151
"tweaks/SpecialMembersTests.cpp",
152+
"tweaks/SwapBinaryOperandsTests.cpp",
152153
"tweaks/SwapIfBranchesTests.cpp",
153154
"tweaks/TweakTesting.cpp",
154155
"tweaks/TweakTests.cpp",

0 commit comments

Comments
 (0)