Skip to content

Commit e83d86f

Browse files
kidq330kidq330
committed
WIP update modernize-replace-memcpy-with-stdcopy implementation
1 parent 3c44177 commit e83d86f

File tree

6 files changed

+324
-78
lines changed

6 files changed

+324
-78
lines changed

‎clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ class ModernizeModule : public ClangTidyModule {
9393
CheckFactories.registerCheck<ReplaceDisallowCopyAndAssignMacroCheck>(
9494
"modernize-replace-disallow-copy-and-assign-macro");
9595
CheckFactories.registerCheck<ReplaceMemcpyWithStdCopy>(
96-
"modernize-replace-memcpy-by-stdcopy");
96+
"modernize-replace-memcpy-with-std-copy");
9797
CheckFactories.registerCheck<ReplaceRandomShuffleCheck>(
9898
"modernize-replace-random-shuffle");
9999
CheckFactories.registerCheck<ReturnBracedInitListCheck>(

‎clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp‎

Lines changed: 255 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -7,113 +7,307 @@
77
//===----------------------------------------------------------------------===//
88

99
#include "ReplaceMemcpyWithStdCopy.h"
10-
#include "../utils/OptionsUtils.h"
10+
#include "../utils/Matchers.h"
11+
#include "clang/AST/DeclarationName.h"
12+
#include "clang/AST/Expr.h"
13+
#include "clang/AST/ExprCXX.h"
14+
#include "clang/AST/Type.h"
15+
#include "clang/ASTMatchers/ASTMatchers.h"
16+
#include "clang/Basic/Diagnostic.h"
17+
#include "clang/Tooling/FixIt.h"
18+
#include "llvm/ADT/StringRef.h"
19+
#include "llvm/Support/ErrorHandling.h"
1120
#include <array>
21+
#include <optional>
22+
#include <type_traits>
23+
#include <variant>
1224

1325
using namespace clang;
1426
using namespace clang::ast_matchers;
1527

16-
namespace clang {
17-
namespace tidy {
18-
namespace modernize {
28+
namespace clang::tidy::modernize {
29+
static constexpr llvm::StringLiteral MemcpyRef = "::std::memcpy";
30+
31+
namespace {
32+
// Helper Matcher which applies the given QualType Matcher either directly or by
33+
// resolving a pointer type to its pointee. Used to match v.push_back() as well
34+
// as p->push_back().
35+
auto hasTypeOrPointeeType(
36+
const ast_matchers::internal::Matcher<QualType> &TypeMatcher) {
37+
return anyOf(hasType(TypeMatcher),
38+
hasType(pointerType(pointee(TypeMatcher))));
39+
}
40+
41+
constexpr llvm::StringLiteral StdCopyNHeader = "<algorithm>";
42+
} // namespace
1943

2044
ReplaceMemcpyWithStdCopy::ReplaceMemcpyWithStdCopy(StringRef Name,
2145
ClangTidyContext *Context)
2246
: ClangTidyCheck(Name, Context),
23-
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
24-
utils::IncludeSorter::IS_LLVM)) {
25-
}
47+
Inserter(Options.getLocalOrGlobal("IncludeStyle",
48+
utils::IncludeSorter::IS_LLVM),
49+
areDiagsSelfContained()) {}
2650

2751
void ReplaceMemcpyWithStdCopy::registerMatchers(MatchFinder *Finder) {
28-
assert(Finder != nullptr);
52+
const auto ByteLikeTypeWithId = [](const char *ID) {
53+
return anything();
54+
// return hasCanonicalType(matchers::isSimpleChar());
55+
};
56+
57+
const auto IsByteSequence = anything();
58+
// hasTypeOrPointeeType(
59+
// hasCanonicalType(hasDeclaration(cxxRecordDecl(hasAnyName({
60+
// "::std::vector",
61+
// "::std::span",
62+
// "::std::deque",
63+
// "::std::array",
64+
// "::std::string",
65+
// "::std::string_view",
66+
// })))));
67+
// __jm__ for template classes need to add a check whether T is ByteLike
68+
// __jm__ these matchers unused because memcpy for non-byte sequences should
69+
// be flagged without FixIt
2970

30-
if (!getLangOpts().CPlusPlus)
31-
return;
71+
const auto IsDestContainerData = cxxMemberCallExpr(
72+
callee(cxxMethodDecl(
73+
hasName("data"),
74+
returns(pointerType(pointee(ByteLikeTypeWithId("data_return_type")))),
75+
unless(isConst()))),
76+
argumentCountIs(0), on(expr(IsByteSequence)));
3277

33-
auto MemcpyMatcher =
34-
callExpr(hasDeclaration(functionDecl(hasName("memcpy"),
35-
isExpansionInSystemHeader())),
36-
isExpansionInMainFile())
37-
.bind("memcpy_function");
78+
const auto IsSourceContainerData = cxxMemberCallExpr(
79+
callee(cxxMethodDecl(
80+
hasName("data"),
81+
returns(pointerType(pointee(ByteLikeTypeWithId("data_return_type"))))
82+
//,isConst() __jm__ doesn't have to be const pre-implicit cast, but
83+
// that would require switching traversal type
84+
)),
85+
argumentCountIs(0),
86+
on(expr(IsByteSequence))); // __jm__ how to express that the caller needs
87+
// to be const qualified / a const lvalue?
3888

39-
Finder->addMatcher(MemcpyMatcher, this);
89+
auto IsDestCArrayOrContainerData =
90+
anyOf(IsDestContainerData.bind("dest_data"),
91+
hasType(hasCanonicalType(arrayType().bind("dest_array"))));
92+
93+
auto IsSourceCArrayOrContainerData =
94+
anyOf(IsSourceContainerData, hasType(hasCanonicalType(arrayType())));
95+
// all of the pointer args to memcpy must be any of:
96+
// 1. .data() call to record which
97+
// can be passed to std::begin or has .begin method and
98+
// can be passed to std::end or has .end method and
99+
// can be passed to std::size or has .size method
100+
// 2. static c-array
101+
const auto ReturnValueUsed =
102+
optionally(hasParent(compoundStmt().bind("return_value_discarded")));
103+
104+
const auto MemcpyDecl =
105+
functionDecl(hasAnyName("::std::memcpy", "::memcpy")
106+
// , argumentCountIs(3) __jm__ doesn't work for
107+
// functionDecl, possibly only for callExpr?
108+
);
109+
// __jm__ also match on arg types
110+
const auto Expression =
111+
callExpr(callee(MemcpyDecl), ReturnValueUsed,
112+
hasArgument(0, expr(IsDestCArrayOrContainerData).bind("dest")),
113+
hasArgument(1, IsSourceCArrayOrContainerData));
114+
Finder->addMatcher(Expression.bind(MemcpyRef), this);
40115
}
41116

42117
void ReplaceMemcpyWithStdCopy::registerPPCallbacks(
43118
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
44-
if (!getLangOpts().CPlusPlus)
45-
return;
119+
Inserter.registerPreprocessor(PP);
120+
}
46121

47-
Inserter =
48-
std::make_unique<utils::IncludeInserter>(SM, getLangOpts(),
49-
IncludeStyle);
50-
PP->addPPCallbacks(Inserter->CreatePPCallbacks());
122+
void ReplaceMemcpyWithStdCopy::storeOptions(ClangTidyOptions::OptionMap &Opts) {
123+
Options.store(Opts, "IncludeStyle", Inserter.getStyle());
51124
}
52125

53-
void ReplaceMemcpyWithStdCopy::check(const MatchFinder::MatchResult &Result) {
54-
const auto *MemcpyNode = Result.Nodes.getNodeAs<CallExpr>("memcpy_function");
55-
assert(MemcpyNode != nullptr);
126+
// Determine if the result of an expression is "stored" in some way.
127+
// It is true if the value is stored into a variable or used as initialization
128+
// or passed to a function or constructor.
129+
// For this use case compound assignments are not counted as a "store" (the 'E'
130+
// expression should have pointer type).
131+
static bool isExprValueStored(const Expr *E, ASTContext &C) {
132+
E = E->IgnoreParenCasts();
133+
// Get first non-paren, non-cast parent.
134+
ParentMapContext &PMap = C.getParentMapContext();
135+
DynTypedNodeList P = PMap.getParents(*E);
136+
if (P.size() != 1)
137+
return false;
138+
const Expr *ParentE = nullptr;
139+
while ((ParentE = P[0].get<Expr>()) && ParentE->IgnoreParenCasts() == E) {
140+
P = PMap.getParents(P[0]);
141+
if (P.size() != 1)
142+
return false;
143+
}
144+
145+
if (const auto *ParentVarD = P[0].get<VarDecl>())
146+
return ParentVarD->getInit()->IgnoreParenCasts() == E;
147+
148+
if (!ParentE)
149+
return false;
56150

57-
DiagnosticBuilder Diag =
58-
diag(MemcpyNode->getExprLoc(), "use std::copy instead of memcpy");
151+
if (const auto *BinOp = dyn_cast<BinaryOperator>(ParentE))
152+
return BinOp->getOpcode() == BO_Assign &&
153+
BinOp->getRHS()->IgnoreParenCasts() == E;
59154

60-
renameFunction(Diag, MemcpyNode);
61-
reorderArgs(Diag, MemcpyNode);
62-
insertHeader(Diag, MemcpyNode, Result.SourceManager);
155+
return isa<CallExpr, CXXConstructExpr>(ParentE);
63156
}
157+
#include "llvm/Support/Debug.h"
158+
#define DEBUG_TYPE "clang-tidy"
64159

65-
void ReplaceMemcpyWithStdCopy::storeOptions(ClangTidyOptions::OptionMap &Opts) {
66-
Options.store(Opts, "IncludeStyle",
67-
utils::IncludeSorter::toString(IncludeStyle));
160+
bool ReplaceMemcpyWithStdCopy::checkIsByteSequence(
161+
const MatchFinder::MatchResult &Result, std::string_view Prefix) {
162+
163+
const auto &ArgNode = *Result.Nodes.getNodeAs<Expr>("dest");
164+
ArgNode.dumpColor();
165+
166+
auto PointerType_ = ArgNode.getType()->getAs<PointerType>();
167+
if (PointerType_ == nullptr)
168+
return false;
169+
170+
auto PointeeType = PointerType_->getPointeeType();
171+
// if
172+
// (StructFieldTy->isIncompleteType())
173+
// return false;
174+
auto PointeeWidth =
175+
Result.Context->getTypeInfo(PointeeType.getTypePtr()).Width;
176+
177+
bool IsArgPtrToBytelike = PointeeWidth == Result.Context->getCharWidth();
178+
179+
LLVM_DEBUG(llvm::dbgs() << "__jm__ dbgs\n");
180+
// ArgNode.dumpPretty(*Result.Context);
181+
ArgNode.dumpColor();
182+
if (ArgNode.IgnoreImplicit()->getType()->isArrayType()) {
183+
LLVM_DEBUG(llvm::dbgs()
184+
<< name<CArray>() << "__jm__ array \n typewidth:" << PointeeWidth
185+
<< "\n charwidth:" << Result.Context->getCharWidth());
186+
187+
return IsArgPtrToBytelike;
188+
}
189+
190+
// ArgNode is a result of either std::data or .data()
191+
if (const auto *MC =
192+
dyn_cast_or_null<CXXMemberCallExpr>(ArgNode.IgnoreCasts());
193+
MC != nullptr) {
194+
// return false;
195+
LLVM_DEBUG(llvm::dbgs() << "__jm__ member call");
196+
// diag(ArgNode.getExprLoc(), "%0 isnull")
197+
// << (MC->getMethodDecl() == nullptr);
198+
} else {
199+
LLVM_DEBUG(llvm::dbgs() << "__jm__ otherwise");
200+
}
201+
return false;
202+
// else if (auto *FC = P->get<CallExpr>(); FC != nullptr) {
203+
// diag(ArgNode.getExprLoc(), "%0 _2 name is")
204+
// <<
205+
// FC->getCalleeDecl()->getCanonicalDecl()->getAsFunction()->getName();
206+
// }
68207
}
69208

70-
void ReplaceMemcpyWithStdCopy::renameFunction(DiagnosticBuilder &Diag,
71-
const CallExpr *MemcpyNode) {
72-
const CharSourceRange FunctionNameSourceRange = CharSourceRange::getCharRange(
73-
MemcpyNode->getBeginLoc(), MemcpyNode->getArg(0)->getBeginLoc());
209+
void ReplaceMemcpyWithStdCopy::check(const MatchFinder::MatchResult &Result) {
210+
const auto *MemcpyNode = Result.Nodes.getNodeAs<CallExpr>(MemcpyRef);
211+
assert(MemcpyNode != nullptr && "Matched node cannot be null");
212+
213+
auto Diag = diag(MemcpyNode->getExprLoc(), "prefer std::copy_n to memcpy");
214+
215+
// don't issue a fixit if the result of the call is used
216+
// if (bool IsMemcpyReturnValueUsed =
217+
// Result.Nodes.getNodeAs<Expr>("return_value_discarded") == nullptr;
218+
// IsMemcpyReturnValueUsed)
219+
// return;
220+
221+
auto *DestArg = MemcpyNode->getArg(0);
222+
auto *SrcArg = MemcpyNode->getArg(1);
223+
auto *SizeArg = MemcpyNode->getArg(2);
224+
225+
assert(DestArg != nullptr and SrcArg != nullptr and SizeArg != nullptr and
226+
"Call node arguments cannot be null");
227+
228+
if (not checkIsByteSequence(Result, "dest")
229+
// or
230+
// not checkIsByteSequence(Result, *SrcArg, Diag)
231+
)
232+
diag(MemcpyNode->getExprLoc(), "not a byte sequence");
233+
234+
// // __jm__ strip .data() / std::data calls
235+
236+
// // const CharSourceRange FunctionNameSourceRange =
237+
// // CharSourceRange::getCharRange(
238+
// // MemcpyNode->getBeginLoc(), MemcpyNode->getArg(0)->getBeginLoc());
239+
// Diag << FixItHint::CreateReplacement(
240+
// MemcpyNode->getCallee()->getSourceRange(), "std::copy_n");
74241

75-
Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, "std::copy(");
242+
{
243+
// using namespace std::literals;
244+
// Diag << FixItHint::CreateReplacement(
245+
// DestArg->getSourceRange(),
246+
// ("std::begin(" +
247+
// tooling::fixit::getText(SrcArg->getSourceRange(), *Result.Context) +
248+
// ")")
249+
// .str());
250+
// Diag << FixItHint::CreateReplacement(
251+
// SrcArg->getSourceRange(),
252+
// tooling::fixit::getText(SizeArg->getSourceRange(), *Result.Context));
253+
// Diag << FixItHint::CreateReplacement(
254+
// SizeArg->getSourceRange(),
255+
// ("std::begin(" +
256+
// tooling::fixit::getText(DestArg->getSourceRange(), *Result.Context)
257+
// +
258+
// ")")
259+
// .str());
260+
}
261+
// dest source size -> source size dest
262+
// renameFunction(Diag, MemcpyNode);
263+
// reorderArgs(Diag, MemcpyNode);
264+
// insertHeader(Diag, MemcpyNode, Result.SourceManager);
265+
266+
Diag << Inserter.createIncludeInsertion(
267+
Result.SourceManager->getFileID(MemcpyNode->getBeginLoc()),
268+
StdCopyNHeader);
76269
}
77270

271+
// void ReplaceMemcpyWithStdCopy::handleMemcpy(const CallExpr &Node) {
272+
273+
// // FixIt
274+
// const CharSourceRange FunctionNameSourceRange =
275+
// CharSourceRange::getCharRange(
276+
// Node.getBeginLoc(), Node.getArg(0)->getBeginLoc());
277+
278+
// Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
279+
// "std::copy_n(");
280+
// }
281+
282+
void ReplaceMemcpyWithStdCopy::renameFunction(DiagnosticBuilder &Diag,
283+
const CallExpr *MemcpyNode) {}
284+
78285
void ReplaceMemcpyWithStdCopy::reorderArgs(DiagnosticBuilder &Diag,
79286
const CallExpr *MemcpyNode) {
80-
std::array<std::string, 3> arg;
287+
std::array<std::string, 3> Arg;
81288

82289
LangOptions LangOpts;
83290
LangOpts.CPlusPlus = true;
84291
PrintingPolicy Policy(LangOpts);
85292

86293
// Retrieve all the arguments
87-
for (uint8_t i = 0; i < arg.size(); i++) {
88-
llvm::raw_string_ostream s(arg[i]);
89-
MemcpyNode->getArg(i)->printPretty(s, nullptr, Policy);
294+
for (uint8_t I = 0; I < Arg.size(); I++) {
295+
llvm::raw_string_ostream S(Arg[I]);
296+
MemcpyNode->getArg(I)->printPretty(S, nullptr, Policy);
90297
}
91298

92299
// Create lambda that return SourceRange of an argument
93-
auto getSourceRange = [MemcpyNode](uint8_t ArgCount) -> SourceRange {
300+
auto GetSourceRange = [MemcpyNode](uint8_t ArgCount) -> SourceRange {
94301
return SourceRange(MemcpyNode->getArg(ArgCount)->getBeginLoc(),
95302
MemcpyNode->getArg(ArgCount)->getEndLoc());
96303
};
97304

98305
// Reorder the arguments
99-
Diag << FixItHint::CreateReplacement(getSourceRange(0), arg[1]);
306+
Diag << FixItHint::CreateReplacement(GetSourceRange(0), Arg[1]);
100307

101-
arg[2] = arg[1] + " + ((" + arg[2] + ") / sizeof(*(" + arg[1] + ")))";
102-
Diag << FixItHint::CreateReplacement(getSourceRange(1), arg[2]);
308+
Arg[2] = Arg[1] + " + ((" + Arg[2] + ") / sizeof(*(" + Arg[1] + ")))";
309+
Diag << FixItHint::CreateReplacement(GetSourceRange(1), Arg[2]);
103310

104-
Diag << FixItHint::CreateReplacement(getSourceRange(2), arg[0]);
311+
Diag << FixItHint::CreateReplacement(GetSourceRange(2), Arg[0]);
105312
}
106-
107-
void ReplaceMemcpyWithStdCopy::insertHeader(DiagnosticBuilder &Diag,
108-
const CallExpr *MemcpyNode,
109-
SourceManager *const SM) {
110-
Optional<FixItHint> FixInclude = Inserter->CreateIncludeInsertion(
111-
/*FileID=*/SM->getMainFileID(), /*Header=*/"algorithm",
112-
/*IsAngled=*/true);
113-
if (FixInclude)
114-
Diag << *FixInclude;
115-
}
116-
117-
} // namespace modernize
118-
} // namespace tidy
119-
} // namespace clang
313+
} // namespace clang::tidy::modernize

0 commit comments

Comments
 (0)