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

[giovannism20]

Closes #22583

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 64454daab0c34d9f3a488979b6b7dfbe315fa9f8 bd8e10820557a538ec1cd93f3f020d286132c688 -- clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp

View the diff from clang-format here.

diff --git a/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp index af6b365c16..fd728e2995 100644 --- a/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp @@ -45,8 +45,7 @@ void ReplaceMemcpyWithStdCopy::registerPPCallbacks( return; Inserter = - std::make_unique<utils::IncludeInserter>(SM, getLangOpts(), - IncludeStyle); + std::make_unique<utils::IncludeInserter>(SM, getLangOpts(), IncludeStyle); PP->addPPCallbacks(Inserter->CreatePPCallbacks()); } 

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Giovanni Martins (giovannism20)

Changes

#22583

---

Full diff: https://github.com/llvm/llvm-project/pull/74663.diff

7 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1)
• (modified) clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp (+3)
• (added) clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp (+119)
• (added) clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h (+49)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
• (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
• (added) clang-tools-extra/docs/clang-tidy/checks/modernize/modernize-replace-memcpy-with-stdcopy.rst (+47)

diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index c40065358d2dc3..d0a996d3be7292 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -22,6 +22,7 @@ add_clang_library(clangTidyModernizeModule RedundantVoidArgCheck.cpp ReplaceAutoPtrCheck.cpp ReplaceDisallowCopyAndAssignMacroCheck.cpp + ReplaceMemcpyWithStdCopy.cpp ReplaceRandomShuffleCheck.cpp ReturnBracedInitListCheck.cpp ShrinkToFitCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index e994ffd2a75c85..6bb9efa694eb2f 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -23,6 +23,7 @@ #include "RedundantVoidArgCheck.h" #include "ReplaceAutoPtrCheck.h" #include "ReplaceDisallowCopyAndAssignMacroCheck.h" +#include "ReplaceMemcpyWithStdCopy.h" #include "ReplaceRandomShuffleCheck.h" #include "ReturnBracedInitListCheck.h" #include "ShrinkToFitCheck.h" @@ -78,6 +79,8 @@ class ModernizeModule : public ClangTidyModule { "modernize-replace-auto-ptr"); CheckFactories.registerCheck<ReplaceDisallowCopyAndAssignMacroCheck>( "modernize-replace-disallow-copy-and-assign-macro"); + CheckFactories.registerCheck<ReplaceMemcpyWithStdCopy>( + "modernize-replace-memcpy-by-stdcopy"); CheckFactories.registerCheck<ReplaceRandomShuffleCheck>( "modernize-replace-random-shuffle"); CheckFactories.registerCheck<ReturnBracedInitListCheck>( diff --git a/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp new file mode 100644 index 00000000000000..af6b365c162517 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp @@ -0,0 +1,119 @@ +//===--- ReplaceMemcpyWithStdCopy.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 "ReplaceMemcpyWithStdCopy.h" +#include "../utils/OptionsUtils.h" +#include <array> + +using namespace clang; +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace modernize { + +ReplaceMemcpyWithStdCopy::ReplaceMemcpyWithStdCopy(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IncludeInserter(Options.getLocalOrGlobal("IncludeStyle", + utils::IncludeSorter::IS_LLVM)) { +} + +void ReplaceMemcpyWithStdCopy::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 ReplaceMemcpyWithStdCopy::registerPPCallbacks( + const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { + if (!getLangOpts().CPlusPlus) + return; + + Inserter = + std::make_unique<utils::IncludeInserter>(SM, getLangOpts(), + IncludeStyle); + PP->addPPCallbacks(Inserter->CreatePPCallbacks()); +} + +void ReplaceMemcpyWithStdCopy::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 ReplaceMemcpyWithStdCopy::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", + utils::IncludeSorter::toString(IncludeStyle)); +} + +void ReplaceMemcpyWithStdCopy::renameFunction(DiagnosticBuilder &Diag, + const CallExpr *MemcpyNode) { + const CharSourceRange FunctionNameSourceRange = CharSourceRange::getCharRange( + MemcpyNode->getBeginLoc(), MemcpyNode->getArg(0)->getBeginLoc()); + + Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, "std::copy("); +} + +void ReplaceMemcpyWithStdCopy::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 < arg.size(); 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 ReplaceMemcpyWithStdCopy::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 diff --git a/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h new file mode 100644 index 00000000000000..0f262bf839af24 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h @@ -0,0 +1,49 @@ +//===--- ReplaceMemcpyWithStdCopy.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_WITH_STDCOPY_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_MEMCPY_WITH_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 with std::copy +class ReplaceMemcpyWithStdCopy : public ClangTidyCheck { +public: + ReplaceMemcpyWithStdCopy(StringRef Name, ClangTidyContext *Context); + ~ReplaceMemcpyWithStdCopy() 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; + utils::IncludeInserter IncludeInserter; + const utils::IncludeSorter::IncludeStyle IncludeStyle; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_MEMCPY_WITH_STDCOPY_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 4ff4494cef5624..2191e418cead24 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -377,6 +377,11 @@ Changes in existing checks <clang-tidy/checks/modernize/use-nullptr>` check by adding option `IgnoredTypes` that can be used to exclude some pointer types. +- New :doc:`modernize-replace-memcpy-with-stdcopy + <clang-tidy/checks/modernize-replace-memcpy-by-stdcopy>` check. + + Replaces all occurrences of the C ``memcpy`` function by ``std::copy``. + - Improved :doc:`modernize-use-std-print <clang-tidy/checks/modernize/use-std-print>` check to accurately generate fixes for reordering arguments. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index df2d5d15238d63..57d13aacb81890 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -275,6 +275,7 @@ Clang-Tidy Checks :doc:`modernize-raw-string-literal <modernize/raw-string-literal>`, "Yes" :doc:`modernize-redundant-void-arg <modernize/redundant-void-arg>`, "Yes" :doc:`modernize-replace-auto-ptr <modernize/replace-auto-ptr>`, "Yes" + :doc:`modernize-replace-memcpy-with-std-copy <modernize/replace-auto-ptr>`, "Yes" :doc:`modernize-replace-disallow-copy-and-assign-macro <modernize/replace-disallow-copy-and-assign-macro>`, "Yes" :doc:`modernize-replace-random-shuffle <modernize/replace-random-shuffle>`, "Yes" :doc:`modernize-return-braced-init-list <modernize/return-braced-init-list>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/modernize-replace-memcpy-with-stdcopy.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/modernize-replace-memcpy-with-stdcopy.rst new file mode 100644 index 00000000000000..922a7f36e7e078 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/modernize-replace-memcpy-with-stdcopy.rst @@ -0,0 +1,47 @@ +.. title:: clang-tidy - modernize-replace-memcpy-with-stdcopy + +modernize-replace-memcpy-with-stdcopy +=================================== + +Replaces all occurrences of the C ``memcpy`` function with ``std::copy`` + +Example: + +.. code-block:: c++ + + /*! + * \param destination Pointer to the destination array where the content is to be copied + * \param source Pointer to the source of data to be copied + * \param num Number of bytes to copy + */ + memcpy(destination, source, num); + +becomes + +.. code-block:: c++ + + /*! + * \param destination Pointer to the destination array where the content is to be copied + * \param source Pointer to the source of data to be copied + * \param num Number of bytes to copy + */ + std::copy(source, source + (num / sizeof *source), destination); + +Bytes to iterator conversion +---------------------------- + +Unlike ``std::copy`` that take an iterator on the last element of the source array, ``memcpy`` request the number of bytes to copy. +In order to make the check working, it will convert the size parameter to an iterator by replacing it by ``source + (num / sizeof *source)`` + +Header inclusion +---------------- + +``std::copy`` being provided by the ``algorithm`` header file, this check will include it if needed. + +Options +------- + +.. option:: IncludeStyle + + A string specifying which include-style is used, `llvm` or `google`. Default + is `llvm`. 

[PiotrZSL]

Missing tests.
Lot of issues.
Personally i would consider modernize-use-std-copy-fill as an name, and cover also memset, as this would be the same code. And I would convert this into std::copy_n or std::full_n.

The only thing when std::copy should eb used would be if call to memset would be:
memset(other_begin, begin, end-begin);

[PiotrZSL]

use

bool isLanguageVersionSupported(const LangOptions &LangOptions) const override { return LangOptions.CPlusPlus; } 

instead

[PiotrZSL]

not needed

[PiotrZSL]

Why you create 2 Include inserters ? create only one.

[PiotrZSL]

IncludeStyle is uninitialized.

[PiotrZSL]

use callee instead of hasDeclaration

[PiotrZSL]

should cover check name

[PiotrZSL]

those doxygen comments arent needed

[PiotrZSL]

again, use std::copy_n

[PiotrZSL]

looks like workaround

[PiotrZSL]

Not needed info