[clang-tidy] Replace memcpy with std::copy #74663
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.cppView 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());
}
|
giovannism20
changed the title
|
@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:
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
requested changes
Dec 9, 2023
There was a problem hiding this comment.
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);
| void ReplaceMemcpyWithStdCopy::registerMatchers(MatchFinder *Finder) { | ||
| assert(Finder != nullptr); | ||
|
|
||
| if (!getLangOpts().CPlusPlus) |
There was a problem hiding this comment.
use
bool
isLanguageVersionSupported(const LangOptions &LangOptions) const override {
return LangOptions.CPlusPlus;
}
instead
| } | ||
|
|
||
| void ReplaceMemcpyWithStdCopy::registerMatchers(MatchFinder *Finder) { | ||
| assert(Finder != nullptr); |
| SourceManager *const SM); | ||
|
|
||
| private: | ||
| std::unique_ptr<utils::IncludeInserter> Inserter; |
There was a problem hiding this comment.
Why you create 2 Include inserters ? create only one.
| private: | ||
| std::unique_ptr<utils::IncludeInserter> Inserter; | ||
| utils::IncludeInserter IncludeInserter; | ||
| const utils::IncludeSorter::IncludeStyle IncludeStyle; |
| return; | ||
|
|
||
| auto MemcpyMatcher = | ||
| callExpr(hasDeclaration(functionDecl(hasName("memcpy"), |
There was a problem hiding this comment.
use callee instead of hasDeclaration
| .. title:: clang-tidy - modernize-replace-memcpy-with-stdcopy | ||
|
|
||
| modernize-replace-memcpy-with-stdcopy | ||
| =================================== |
|
|
||
| .. code-block:: c++ | ||
|
|
||
| /*! |
There was a problem hiding this comment.
those doxygen comments arent needed
| * \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. |
Comment on lines
+36
to
+39
| Header inclusion | ||
| ---------------- | ||
|
|
||
| ``std::copy`` being provided by the ``algorithm`` header file, this check will include it if needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #22583