[clang-tidy] Replace memcpy by std::copy

[Blackhart]

Summary

This patch will:

• replace all occurrence of the C "memcpy" function by "std::copy".
• reorder its arguments.
• insert the "algorithm" header if needed.

[lebedev.ri]

Missing tests.

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp

119

please add all the missing newlines

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.h

2–3

should be something like

//===--- ReplaceMemcpyByStdCopy.h - clang-tidy -----------------*- C++-*-===//

[Blackhart]

Add missing "override" keywords

[Blackhart]

Fix file comments typo

[Blackhart]

Modernize memcpy only if C++20 is enabled

[lebedev.ri]

In D63324#1543607, @Blackhart wrote:

Modernize memcpy only if C++20 is enabled

... why?
This is also missing documentation,releasenotes changes.

[Blackhart]

In D63324#1543609, @lebedev.ri wrote:

In D63324#1543607, @Blackhart wrote:

Modernize memcpy only if C++20 is enabled

... why?
This is also missing documentation,releasenotes changes.

According with the C++ reference, std::copy is only available since C++20.
There is another std::copy signature available since C++17, but it needs an extra parameter. I can implement it also.
https://en.cppreference.com/w/cpp/algorithm/copy

I'm working on the unit tests and I'll make documentation/releasenotes after that.

Thanks for reviewing @lebedev.ri ;)

[lebedev.ri]

In D63324#1543626, @Blackhart wrote:

In D63324#1543609, @lebedev.ri wrote:

In D63324#1543607, @Blackhart wrote:

Modernize memcpy only if C++20 is enabled

... why?
This is also missing documentation,releasenotes changes.

According with the C++ reference, std::copy is only available since C++20.

I don't see it there, can you quote?
https://godbolt.org/z/q2ryJi

There is another std::copy signature available since C++17, but it needs an extra parameter. I can implement it also.
https://en.cppreference.com/w/cpp/algorithm/copy

I'm working on the unit tests and I'll make documentation/releasenotes after that.

Thanks for reviewing @lebedev.ri ;)

[xbolva00]

This might not be currently ideal recommendation since std::copy produces memmove with -O3.

[Eugene.Zelenko]

New check must be mentioned in Release Notes and its documentation provided.

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp

69

Return type could not be deduced from same statement, so please don't use auto.

77

Will be good idea to use std::array and its size instead of numerical constant.

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.h

27

Should be override and = default. See modernize-use-override and modernize-use-equals-default.

[Blackhart]

In D63324#1543706, @lebedev.ri wrote:

In D63324#1543626, @Blackhart wrote:

In D63324#1543609, @lebedev.ri wrote:

In D63324#1543607, @Blackhart wrote:

Modernize memcpy only if C++20 is enabled

... why?
This is also missing documentation,releasenotes changes.

According with the C++ reference, std::copy is only available since C++20.

I don't see it there, can you quote?
https://godbolt.org/z/q2ryJi

There is another std::copy signature available since C++17, but it needs an extra parameter. I can implement it also.
https://en.cppreference.com/w/cpp/algorithm/copy

I'm working on the unit tests and I'll make documentation/releasenotes after that.

Thanks for reviewing @lebedev.ri ;)

Sorry, I misread the reference. It's "until" C++20.
So from C++ birth to today ...

I'll revert my changes.

[Blackhart]

In D63324#1543782, @xbolva00 wrote:

This might not be currently ideal recommendation since std::copy produces memmove with -O3.

What do you recommend ?
There is a way to disable this modernization if the user compile with the -O3 flag ?

[Blackhart]

Revert "Modernize memcpy only if C++20 is enabled"

[Blackhart]

Add override keyword to the destructor and let the compiler generate its definition

[Blackhart]

Remove "auto" keyword when type is created from same statement

[Blackhart]

Use "std::array" instead of the raw array

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp

11

Please remove unnecessary empty line.

86

Please use arg,size() instead of 3.

[Blackhart]

• Remove unnessecary empty line
• Use "array.size()" instead of numerical constant

[Blackhart]

• Update releasenotes
• Add documentation

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.h

42

I think this could be determined from .clang-format.

clang-tools-extra/docs/ReleaseNotes.rst

204 ↗

(On Diff #204932)

Please move into new checks list (in alphabetical order).

207 ↗

(On Diff #204932)

Replaces. Please use double back-ticks to highlight language constructs.

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-memcpy-by-stdcopy.rst

6 ↗

(On Diff #204932)

Please make first sentance same as in Release Notes.

35 ↗

(On Diff #204932)

I think this could be determined from project's .clang-format.

[Blackhart]

• Reorder release note changes
• Update documentation

[Blackhart]

Hello,

I've updated the documentation with more informations about the transformations the check do.

I'm not very confident in writing documentation in english.
Please review it and submit me your change requests.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

192 ↗

(On Diff #205331)

Please remove This check. Same in first statement in documentation.

[lebedev.ri]

Still missing tests

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp

35–36

Why these restrictions?

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.h

27

I don't think you need this one

[Blackhart]

Remove "this check" from documentation and release note

[JonasToth]

You need tests for this check. Please harden them against macro-interference as well. I think the transformation should happen as function call that return Optional and if any failure happens the diagnostic can still be emitted.

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp

28

this assertion is not necessary. Finder is always not-null.

34

please match on ::memcpy and ::std::memcpy.

54

this assertion is not necessary, as you only match on one thing and that means it exists. (otherwise everything is broken already).

[lebedev.ri]

Thinking about it more, i have some negative reservations about this :/

As it can be seen form https://godbolt.org/z/3BZmCM, it seems any and every(?) alternative C++ algorithm
replacement is a performance pessimization in the general case, because memcpy requires/'guarantees'
that there must be no overlap between source and destination ranges,
while there is no such restriction for C++ algorithms (=> they will get optimized into memmove).
That is unless optimizer also manages to prove that it is safe to optimize memmove into memcpy there.
(Extra bloat if(num != 0) memmove is also not always good)

[xbolva00]

if(num != 0) memmove is missed optimization opportunity. Such branch can be removed (if profitable).

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

19

In English, it's more idiomatic to say "Replace memcpy with std::copy",
but perhaps an even better name for this check is modernize-use-std-copy.
This opens the door to future improvements that recognize hand-written
for loops that are essentially just invocations of std::copy or std::copy_n.
(Recently I was considering writing a check like that while reviewing some
code at work that was full of hand-written for loops that just copied elements
from one container to another.)

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp

11

Please remove unnecessary empty line.

Where is this in the LLVM coding style guide?

36

I erroneously had this in some checks I had written; it prevents fixits
from being applied to user supplied header files and is undesirable.

I would suggest removing both the isExpansionInSystemHeader and
isExpansionInMainFile narrowing matchers.

71

In the clang code base, they prefer not to use const on local variables
unless the local is a pointer to a const object and even then, it's only
the pointed-to object that is declared const not the pointer itself.

79

LLVM naming convention is CapCamelCase for variables.
See here and variables declared on lines 85, 86, 91, etc.

92

Prefer int over uint8_t here as there is no requirement for
a type of a specific bit width. The explicit return type is also
unnecessary, but please consult the LLVM style guide to make
sure you're following any prescriptions for lambdas.

Additionally, ArgCount doesn't feel like the right name for this
parameter. It's not the count of anything, it's an index into the
arguments, so just Arg feels better.

100

I think it would be worthwhile to probe the AST of the argument here and see if it is
already of the form N*sizeof(T).

This can probably be handled by enhancing the matchers on the argument so that you
aren't probing the AST in procedural code.

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.h

27

Should be override and = default

Why do you need to override it if you're just using the compiler's default?

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-memcpy-by-stdcopy.rst

33 ↗

(On Diff #205354)

Please use a line length in the .rst files that is consistent with the LLVM line length
for code files.

clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   MakeSharedCheck.cpp
   MakeSmartPtrCheck.cpp
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
+  ReplaceMemcpyByStdCopy.cpp
   ReplaceRandomShuffleCheck.cpp
   ReturnBracedInitListCheck.cpp
   ShrinkToFitCheck.cpp
   UnaryStaticAssertCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp

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

 #include "DeprecatedIosBaseAliasesCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
+#include "ReplaceMemcpyByStdCopy.h"
 #include "ReplaceRandomShuffleCheck.h"
 #include "ReturnBracedInitListCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UnaryStaticAssertCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique");
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<RawStringLiteralCheck>(
         "modernize-raw-string-literal");
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
+    CheckFactories.registerCheck<ReplaceMemcpyByStdCopy>(
+        "modernize-replace-memcpy-by-stdcopy");
     CheckFactories.registerCheck<ReplaceRandomShuffleCheck>(
         "modernize-replace-random-shuffle");
     CheckFactories.registerCheck<ReturnBracedInitListCheck>(
         "modernize-return-braced-init-list");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     CheckFactories.registerCheck<UnaryStaticAssertCheck>(
         "modernize-unary-static-assert");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.h

+//===--- ReplaceMemcpyByStdCopy.h - clang-tidy--------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_MEMCPY_BY_STDCOPY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_MEMCPY_BY_STDCOPY_H
+
+#include "../ClangTidyCheck.h"
+#include "../utils/IncludeInserter.h"
+#include <memory>
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replace the C memcpy function by std::copy
+class ReplaceMemcpyByStdCopy : public ClangTidyCheck {
+public:
+  ReplaceMemcpyByStdCopy(StringRef Name, ClangTidyContext *Context);
+  ~ReplaceMemcpyByStdCopy() override = default;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+
+private:
+  void renameFunction(DiagnosticBuilder &Diag, const CallExpr *MemcpyNode);
+  void reorderArgs(DiagnosticBuilder &Diag, const CallExpr *MemcpyNode);
+  void insertHeader(DiagnosticBuilder &Diag, const CallExpr *MemcpyNode,
+                    SourceManager *const SM);
+
+private:
+  std::unique_ptr<utils::IncludeInserter> Inserter;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_MEMCPY_BY_STDCOPY_H

clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp

+//===--- ReplaceMemcpyByStdCopy.cpp - clang-tidy------------------*- 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 "ReplaceMemcpyByStdCopy.h"
+#include "../utils/OptionsUtils.h"
+
+#include <array>
+
+using namespace clang;
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+ReplaceMemcpyByStdCopy::ReplaceMemcpyByStdCopy(StringRef Name,
+                                               ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+          Options.getLocalOrGlobal("IncludeStyle", "llvm"))) {}
+
+void ReplaceMemcpyByStdCopy::registerMatchers(MatchFinder *Finder) {
+  assert(Finder != nullptr);
+
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  auto MemcpyMatcher =
+      callExpr(hasDeclaration(functionDecl(hasName("memcpy"),
+                                           isExpansionInSystemHeader())),
+               isExpansionInMainFile())
+          .bind("memcpy_function");
+
+  Finder->addMatcher(MemcpyMatcher, this);
+}
+
+void ReplaceMemcpyByStdCopy::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Inserter = llvm::make_unique<utils::IncludeInserter>(SM, getLangOpts(),
+                                                       IncludeStyle);
+  PP->addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void ReplaceMemcpyByStdCopy::check(const MatchFinder::MatchResult &Result) {
+  const auto *MemcpyNode = Result.Nodes.getNodeAs<CallExpr>("memcpy_function");
+  assert(MemcpyNode != nullptr);
+
+  DiagnosticBuilder Diag =
+      diag(MemcpyNode->getExprLoc(), "use std::copy instead of memcpy");
+
+  renameFunction(Diag, MemcpyNode);
+  reorderArgs(Diag, MemcpyNode);
+  insertHeader(Diag, MemcpyNode, Result.SourceManager);
+}
+
+void ReplaceMemcpyByStdCopy::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle",
+                utils::IncludeSorter::toString(IncludeStyle));
+}
+
+void ReplaceMemcpyByStdCopy::renameFunction(DiagnosticBuilder &Diag,
+                                            const CallExpr *MemcpyNode) {
+  const CharSourceRange FunctionNameSourceRange = CharSourceRange::getCharRange(
+      MemcpyNode->getBeginLoc(), MemcpyNode->getArg(0)->getBeginLoc());
+
+  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, "std::copy(");
+}
+
+void ReplaceMemcpyByStdCopy::reorderArgs(DiagnosticBuilder &Diag,
+                                         const CallExpr *MemcpyNode) {
+  std::array<std::string, 3> arg;
+
+  LangOptions LangOpts;
+  LangOpts.CPlusPlus = true;
+  PrintingPolicy Policy(LangOpts);
+
+  // Retrieve all the arguments
+  for (uint8_t i = 0; i < 3; i++) {
+    llvm::raw_string_ostream s(arg[i]);
+    MemcpyNode->getArg(i)->printPretty(s, nullptr, Policy);
+  }
+
+  // Create lambda that return SourceRange of an argument
+  auto getSourceRange = [MemcpyNode](uint8_t ArgCount) -> SourceRange {
+    return SourceRange(MemcpyNode->getArg(ArgCount)->getBeginLoc(),
+                       MemcpyNode->getArg(ArgCount)->getEndLoc());
+  };
+
+  // Reorder the arguments
+  Diag << FixItHint::CreateReplacement(getSourceRange(0), arg[1]);
+
+  arg[2] = arg[1] + " + ((" + arg[2] + ") / sizeof(*(" + arg[1] + ")))";
+  Diag << FixItHint::CreateReplacement(getSourceRange(1), arg[2]);
+
+  Diag << FixItHint::CreateReplacement(getSourceRange(2), arg[0]);
+}
+
+void ReplaceMemcpyByStdCopy::insertHeader(DiagnosticBuilder &Diag,
+                                          const CallExpr *MemcpyNode,
+                                          SourceManager *const SM) {
+  Optional<FixItHint> FixInclude = Inserter->CreateIncludeInsertion(
+      /*FileID=*/SM->getMainFileID(), /*Header=*/"algorithm",
+      /*IsAngled=*/true);
+  if (FixInclude)
+    Diag << *FixInclude;
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang