https://github.com/kidq330 created https://github.com/llvm/llvm-project/pull/113046
None From b2753fc81792cf5c3a95eedbf349bac2e461e883 Mon Sep 17 00:00:00 2001 From: Giovanni Martins <giovannimartins2...@gmail.com> Date: Wed, 6 Dec 2023 11:26:53 -0300 Subject: [PATCH 1/3] replace memcpy with std::copy on clang-tidy removed typo on files sort imports removed some typo solve linter reports update modernize-replace-memcpy-with-stdcopy.rst --- .../clang-tidy/modernize/CMakeLists.txt | 1 + .../modernize/ModernizeTidyModule.cpp | 3 + .../modernize/ReplaceMemcpyWithStdCopy.cpp | 119 ++++++++++++++++++ .../modernize/ReplaceMemcpyWithStdCopy.h | 49 ++++++++ clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../docs/clang-tidy/checks/list.rst | 1 + .../modernize-replace-memcpy-with-stdcopy.rst | 47 +++++++ 7 files changed, 225 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp create mode 100644 clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/modernize/modernize-replace-memcpy-with-stdcopy.rst diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index c919d49b42873a..83ebd2f12775e7 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -23,6 +23,7 @@ add_clang_library(clangTidyModernizeModule STATIC 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 18607593320635..257c9373444761 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -24,6 +24,7 @@ #include "RedundantVoidArgCheck.h" #include "ReplaceAutoPtrCheck.h" #include "ReplaceDisallowCopyAndAssignMacroCheck.h" +#include "ReplaceMemcpyWithStdCopy.h" #include "ReplaceRandomShuffleCheck.h" #include "ReturnBracedInitListCheck.h" #include "ShrinkToFitCheck.h" @@ -91,6 +92,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 e8148e06b6af28..2d14f5ec913239 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -217,6 +217,11 @@ Changes in existing checks member function calls too and to only expand macros starting with ``PRI`` and ``__PRI`` from ``<inttypes.h>`` in the format string. +- 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 support replacing member function calls too and to only expand macros starting with ``PRI`` diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 0082234f5ed31b..de8197f20b39a4 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -286,6 +286,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`. From 3c441775ba1b544c61355c4c2de28c1a00355154 Mon Sep 17 00:00:00 2001 From: kidp330 <j.migda...@gmail.com> Date: Sat, 19 Oct 2024 16:49:27 +0200 Subject: [PATCH 2/3] Add unit tests --- .../replace-memcpy-with-std-copy.cpp | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/replace-memcpy-with-std-copy.cpp diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/replace-memcpy-with-std-copy.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/replace-memcpy-with-std-copy.cpp new file mode 100644 index 00000000000000..ce07fc5801da6a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/replace-memcpy-with-std-copy.cpp @@ -0,0 +1,146 @@ +// RUN: %check_clang_tidy %s modernize-replace-memcpy-with-std-copy %t + +// CHECK-FIXES: #include <algorithm> + +namespace { + +using size_t = decltype(sizeof(int)); + +namespace std { +typedef long long int64_t; +typedef short int16_t; +typedef char int8_t; + +void *memcpy(void *__restrict dest, const void *__restrict src, size_t n); + +template <typename T> struct vector { + vector(size_t); + + T *data(); + size_t size() const; + void resize(size_t); + using value_type = T; +}; + +size_t size(void *); + +size_t strlen(const char *); +} // namespace std + +void *memcpy(void *__restrict dest, const void *__restrict src, size_t n); +} // namespace + +void notSupportedEx() { + char source[] = "once upon a daydream...", dest[4]; + + auto *primitiveDest = new std::int8_t; + std::memcpy(primitiveDest, source, sizeof primitiveDest); + + auto *primitiveDest2 = new std::int16_t; + std::memcpy(primitiveDest2, source, sizeof primitiveDest); + std::memcpy(primitiveDest2, source, sizeof primitiveDest2); + + double d = 0.1; + std::int64_t n; + // don't warn on calls over non-sequences + std::memcpy(&n, &d, sizeof d); + + // object creation in destination buffer + struct S { + int x{42}; + void *operator new(size_t, void *) noexcept { return nullptr; } + } s; + alignas(S) char buf[sizeof(S)]; + S *ps = new (buf) S; // placement new + // // don't warn on calls over non-sequences + std::memcpy(ps, &s, sizeof s); + + const char *pSource = "once upon a daydream..."; + char *pDest = new char[4]; + std::memcpy(dest, pSource, sizeof dest); + std::memcpy(pDest, source, 4); +} + +void noFixItEx() { + char source[] = "once upon a daydream...", dest[4]; + + // no FixIt when return value is used + auto *ptr = std::memcpy(dest, source, sizeof dest); + // CHECK-MESSAGES: [[@LINE-1]]:20: warning: prefer std::copy_n to memcpy + + std::vector<std::int16_t> vec_i16(4); + // not a supported type, should be a sequence of bytes, otherwise it is difficult to compute the n in copy_n + std::memcpy(vec_i16.data(), source, + vec_i16.size() * sizeof(decltype(vec_i16)::value_type)); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy +} + +void sequenceOfBytesEx() { + // the check should support memcpy conversion for the following types: + // T[] + // std::vector<T> + // std::span<T> + // std::deque<T> + // std::array<T, _> + // std::string + // std::string_view + // where T is byte-like + + char source[] = "once upon a daydream...", dest[4]; + std::memcpy(dest, source, sizeof dest); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(std::begin(source), std::size(dest), std::begin(dest)); + + // __jm__ warn on global call as well + memcpy(dest, source, sizeof dest); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(std::begin(source), std::size(dest), std::begin(dest)); + + std::vector<char> vec_i8(4); + std::memcpy(vec_i8.data(), source, vec_i8.size()); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(std::begin(source), std::size(vec_i8), std::begin(vec_i8)); + + // __jm__ make configurable whether stl containers should use members or free fns. + // __jm__ for now use free fns. only + + std::memcpy(dest, vec_i8.data(), vec_i8.size()); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(std::begin(vec_i8), std::size(vec_i8), std::begin(dest)); + std::memcpy(dest, vec_i8.data(), sizeof(dest)); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(vec_i8.data(), std::size(dest), std::begin(dest)); + std::memcpy(dest, vec_i8.data(), std::size(dest)); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(std::begin(source), std::size(vec_i8), std::begin(vec_i8)); + + std::memcpy(dest, vec_i8.data(), 1 + vec_i8.size() / 2); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(std::begin(vec_i8), 1 + std::size(vec_i8) / 2, std::begin(dest)); + std::memcpy(dest, vec_i8.data(), 1 + sizeof(dest) / 2); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(vec_i8.data(), 1 + std::size(dest) / 2, std::begin(dest)); + std::memcpy(dest, vec_i8.data(), 1 + std::size(dest) / 2); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: prefer std::copy_n to memcpy + // CHECK-FIXES: std::copy_n(std::begin(source), 1 + std::size(dest) / 2, std::begin(vec_i8)); +} + +// void uninitialized_copy_ex() { +// std::vector<std::string> v = {"This", "is", "an", "example"}; + +// std::string* p; +// std::size_t sz; +// std::tie(p, sz) = std::get_temporary_buffer<std::string>(v.size()); +// sz = std::min(sz, v.size()); + +// std::uninitialized_copy_n(v.begin(), sz, p); + +// for (std::string* i = p; i != p + sz; ++i) +// { +// std::cout << *i << ' '; +// i->~basic_string<char>(); +// } +// std::cout << '\n'; + +// std::return_temporary_buffer(p); +// } \ No newline at end of file From e83d86fefcb434abb08169f62e2e9842f7ea4f46 Mon Sep 17 00:00:00 2001 From: kidp330 <j.migda...@gmail.com> Date: Sat, 19 Oct 2024 17:06:47 +0200 Subject: [PATCH 3/3] WIP update modernize-replace-memcpy-with-stdcopy implementation --- .../modernize/ModernizeTidyModule.cpp | 2 +- .../modernize/ReplaceMemcpyWithStdCopy.cpp | 316 ++++++++++++++---- .../modernize/ReplaceMemcpyWithStdCopy.h | 31 +- clang-tools-extra/docs/ReleaseNotes.rst | 4 +- .../docs/clang-tidy/checks/list.rst | 2 +- .../modernize/replace-memcpy-with-stdcopy.rst | 47 +++ 6 files changed, 324 insertions(+), 78 deletions(-) create mode 100644 clang-tools-extra/docs/clang-tidy/checks/modernize/replace-memcpy-with-stdcopy.rst diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index 257c9373444761..b36248ce1411d8 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -93,7 +93,7 @@ class ModernizeModule : public ClangTidyModule { CheckFactories.registerCheck<ReplaceDisallowCopyAndAssignMacroCheck>( "modernize-replace-disallow-copy-and-assign-macro"); CheckFactories.registerCheck<ReplaceMemcpyWithStdCopy>( - "modernize-replace-memcpy-by-stdcopy"); + "modernize-replace-memcpy-with-std-copy"); 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 index af6b365c162517..eb7b89e89f5a36 100644 --- a/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.cpp @@ -7,113 +7,307 @@ //===----------------------------------------------------------------------===// #include "ReplaceMemcpyWithStdCopy.h" -#include "../utils/OptionsUtils.h" +#include "../utils/Matchers.h" +#include "clang/AST/DeclarationName.h" +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" +#include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Tooling/FixIt.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/ErrorHandling.h" #include <array> +#include <optional> +#include <type_traits> +#include <variant> using namespace clang; using namespace clang::ast_matchers; -namespace clang { -namespace tidy { -namespace modernize { +namespace clang::tidy::modernize { +static constexpr llvm::StringLiteral MemcpyRef = "::std::memcpy"; + +namespace { +// Helper Matcher which applies the given QualType Matcher either directly or by +// resolving a pointer type to its pointee. Used to match v.push_back() as well +// as p->push_back(). +auto hasTypeOrPointeeType( + const ast_matchers::internal::Matcher<QualType> &TypeMatcher) { + return anyOf(hasType(TypeMatcher), + hasType(pointerType(pointee(TypeMatcher)))); +} + +constexpr llvm::StringLiteral StdCopyNHeader = "<algorithm>"; +} // namespace ReplaceMemcpyWithStdCopy::ReplaceMemcpyWithStdCopy(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), - IncludeInserter(Options.getLocalOrGlobal("IncludeStyle", - utils::IncludeSorter::IS_LLVM)) { -} + Inserter(Options.getLocalOrGlobal("IncludeStyle", + utils::IncludeSorter::IS_LLVM), + areDiagsSelfContained()) {} void ReplaceMemcpyWithStdCopy::registerMatchers(MatchFinder *Finder) { - assert(Finder != nullptr); + const auto ByteLikeTypeWithId = [](const char *ID) { + return anything(); + // return hasCanonicalType(matchers::isSimpleChar()); + }; + + const auto IsByteSequence = anything(); + // hasTypeOrPointeeType( + // hasCanonicalType(hasDeclaration(cxxRecordDecl(hasAnyName({ + // "::std::vector", + // "::std::span", + // "::std::deque", + // "::std::array", + // "::std::string", + // "::std::string_view", + // }))))); + // __jm__ for template classes need to add a check whether T is ByteLike + // __jm__ these matchers unused because memcpy for non-byte sequences should + // be flagged without FixIt - if (!getLangOpts().CPlusPlus) - return; + const auto IsDestContainerData = cxxMemberCallExpr( + callee(cxxMethodDecl( + hasName("data"), + returns(pointerType(pointee(ByteLikeTypeWithId("data_return_type")))), + unless(isConst()))), + argumentCountIs(0), on(expr(IsByteSequence))); - auto MemcpyMatcher = - callExpr(hasDeclaration(functionDecl(hasName("memcpy"), - isExpansionInSystemHeader())), - isExpansionInMainFile()) - .bind("memcpy_function"); + const auto IsSourceContainerData = cxxMemberCallExpr( + callee(cxxMethodDecl( + hasName("data"), + returns(pointerType(pointee(ByteLikeTypeWithId("data_return_type")))) + //,isConst() __jm__ doesn't have to be const pre-implicit cast, but + // that would require switching traversal type + )), + argumentCountIs(0), + on(expr(IsByteSequence))); // __jm__ how to express that the caller needs + // to be const qualified / a const lvalue? - Finder->addMatcher(MemcpyMatcher, this); + auto IsDestCArrayOrContainerData = + anyOf(IsDestContainerData.bind("dest_data"), + hasType(hasCanonicalType(arrayType().bind("dest_array")))); + + auto IsSourceCArrayOrContainerData = + anyOf(IsSourceContainerData, hasType(hasCanonicalType(arrayType()))); + // all of the pointer args to memcpy must be any of: + // 1. .data() call to record which + // can be passed to std::begin or has .begin method and + // can be passed to std::end or has .end method and + // can be passed to std::size or has .size method + // 2. static c-array + const auto ReturnValueUsed = + optionally(hasParent(compoundStmt().bind("return_value_discarded"))); + + const auto MemcpyDecl = + functionDecl(hasAnyName("::std::memcpy", "::memcpy") + // , argumentCountIs(3) __jm__ doesn't work for + // functionDecl, possibly only for callExpr? + ); + // __jm__ also match on arg types + const auto Expression = + callExpr(callee(MemcpyDecl), ReturnValueUsed, + hasArgument(0, expr(IsDestCArrayOrContainerData).bind("dest")), + hasArgument(1, IsSourceCArrayOrContainerData)); + Finder->addMatcher(Expression.bind(MemcpyRef), this); } void ReplaceMemcpyWithStdCopy::registerPPCallbacks( const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { - if (!getLangOpts().CPlusPlus) - return; + Inserter.registerPreprocessor(PP); +} - Inserter = - std::make_unique<utils::IncludeInserter>(SM, getLangOpts(), - IncludeStyle); - PP->addPPCallbacks(Inserter->CreatePPCallbacks()); +void ReplaceMemcpyWithStdCopy::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", Inserter.getStyle()); } -void ReplaceMemcpyWithStdCopy::check(const MatchFinder::MatchResult &Result) { - const auto *MemcpyNode = Result.Nodes.getNodeAs<CallExpr>("memcpy_function"); - assert(MemcpyNode != nullptr); +// Determine if the result of an expression is "stored" in some way. +// It is true if the value is stored into a variable or used as initialization +// or passed to a function or constructor. +// For this use case compound assignments are not counted as a "store" (the 'E' +// expression should have pointer type). +static bool isExprValueStored(const Expr *E, ASTContext &C) { + E = E->IgnoreParenCasts(); + // Get first non-paren, non-cast parent. + ParentMapContext &PMap = C.getParentMapContext(); + DynTypedNodeList P = PMap.getParents(*E); + if (P.size() != 1) + return false; + const Expr *ParentE = nullptr; + while ((ParentE = P[0].get<Expr>()) && ParentE->IgnoreParenCasts() == E) { + P = PMap.getParents(P[0]); + if (P.size() != 1) + return false; + } + + if (const auto *ParentVarD = P[0].get<VarDecl>()) + return ParentVarD->getInit()->IgnoreParenCasts() == E; + + if (!ParentE) + return false; - DiagnosticBuilder Diag = - diag(MemcpyNode->getExprLoc(), "use std::copy instead of memcpy"); + if (const auto *BinOp = dyn_cast<BinaryOperator>(ParentE)) + return BinOp->getOpcode() == BO_Assign && + BinOp->getRHS()->IgnoreParenCasts() == E; - renameFunction(Diag, MemcpyNode); - reorderArgs(Diag, MemcpyNode); - insertHeader(Diag, MemcpyNode, Result.SourceManager); + return isa<CallExpr, CXXConstructExpr>(ParentE); } +#include "llvm/Support/Debug.h" +#define DEBUG_TYPE "clang-tidy" -void ReplaceMemcpyWithStdCopy::storeOptions(ClangTidyOptions::OptionMap &Opts) { - Options.store(Opts, "IncludeStyle", - utils::IncludeSorter::toString(IncludeStyle)); +bool ReplaceMemcpyWithStdCopy::checkIsByteSequence( + const MatchFinder::MatchResult &Result, std::string_view Prefix) { + + const auto &ArgNode = *Result.Nodes.getNodeAs<Expr>("dest"); + ArgNode.dumpColor(); + + auto PointerType_ = ArgNode.getType()->getAs<PointerType>(); + if (PointerType_ == nullptr) + return false; + + auto PointeeType = PointerType_->getPointeeType(); + // if + // (StructFieldTy->isIncompleteType()) + // return false; + auto PointeeWidth = + Result.Context->getTypeInfo(PointeeType.getTypePtr()).Width; + + bool IsArgPtrToBytelike = PointeeWidth == Result.Context->getCharWidth(); + + LLVM_DEBUG(llvm::dbgs() << "__jm__ dbgs\n"); + // ArgNode.dumpPretty(*Result.Context); + ArgNode.dumpColor(); + if (ArgNode.IgnoreImplicit()->getType()->isArrayType()) { + LLVM_DEBUG(llvm::dbgs() + << name<CArray>() << "__jm__ array \n typewidth:" << PointeeWidth + << "\n charwidth:" << Result.Context->getCharWidth()); + + return IsArgPtrToBytelike; + } + + // ArgNode is a result of either std::data or .data() + if (const auto *MC = + dyn_cast_or_null<CXXMemberCallExpr>(ArgNode.IgnoreCasts()); + MC != nullptr) { + // return false; + LLVM_DEBUG(llvm::dbgs() << "__jm__ member call"); + // diag(ArgNode.getExprLoc(), "%0 isnull") + // << (MC->getMethodDecl() == nullptr); + } else { + LLVM_DEBUG(llvm::dbgs() << "__jm__ otherwise"); + } + return false; + // else if (auto *FC = P->get<CallExpr>(); FC != nullptr) { + // diag(ArgNode.getExprLoc(), "%0 _2 name is") + // << + // FC->getCalleeDecl()->getCanonicalDecl()->getAsFunction()->getName(); + // } } -void ReplaceMemcpyWithStdCopy::renameFunction(DiagnosticBuilder &Diag, - const CallExpr *MemcpyNode) { - const CharSourceRange FunctionNameSourceRange = CharSourceRange::getCharRange( - MemcpyNode->getBeginLoc(), MemcpyNode->getArg(0)->getBeginLoc()); +void ReplaceMemcpyWithStdCopy::check(const MatchFinder::MatchResult &Result) { + const auto *MemcpyNode = Result.Nodes.getNodeAs<CallExpr>(MemcpyRef); + assert(MemcpyNode != nullptr && "Matched node cannot be null"); + + auto Diag = diag(MemcpyNode->getExprLoc(), "prefer std::copy_n to memcpy"); + + // don't issue a fixit if the result of the call is used + // if (bool IsMemcpyReturnValueUsed = + // Result.Nodes.getNodeAs<Expr>("return_value_discarded") == nullptr; + // IsMemcpyReturnValueUsed) + // return; + + auto *DestArg = MemcpyNode->getArg(0); + auto *SrcArg = MemcpyNode->getArg(1); + auto *SizeArg = MemcpyNode->getArg(2); + + assert(DestArg != nullptr and SrcArg != nullptr and SizeArg != nullptr and + "Call node arguments cannot be null"); + + if (not checkIsByteSequence(Result, "dest") + // or + // not checkIsByteSequence(Result, *SrcArg, Diag) + ) + diag(MemcpyNode->getExprLoc(), "not a byte sequence"); + + // // __jm__ strip .data() / std::data calls + + // // const CharSourceRange FunctionNameSourceRange = + // // CharSourceRange::getCharRange( + // // MemcpyNode->getBeginLoc(), MemcpyNode->getArg(0)->getBeginLoc()); + // Diag << FixItHint::CreateReplacement( + // MemcpyNode->getCallee()->getSourceRange(), "std::copy_n"); - Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, "std::copy("); + { + // using namespace std::literals; + // Diag << FixItHint::CreateReplacement( + // DestArg->getSourceRange(), + // ("std::begin(" + + // tooling::fixit::getText(SrcArg->getSourceRange(), *Result.Context) + + // ")") + // .str()); + // Diag << FixItHint::CreateReplacement( + // SrcArg->getSourceRange(), + // tooling::fixit::getText(SizeArg->getSourceRange(), *Result.Context)); + // Diag << FixItHint::CreateReplacement( + // SizeArg->getSourceRange(), + // ("std::begin(" + + // tooling::fixit::getText(DestArg->getSourceRange(), *Result.Context) + // + + // ")") + // .str()); + } + // dest source size -> source size dest + // renameFunction(Diag, MemcpyNode); + // reorderArgs(Diag, MemcpyNode); + // insertHeader(Diag, MemcpyNode, Result.SourceManager); + + Diag << Inserter.createIncludeInsertion( + Result.SourceManager->getFileID(MemcpyNode->getBeginLoc()), + StdCopyNHeader); } +// void ReplaceMemcpyWithStdCopy::handleMemcpy(const CallExpr &Node) { + +// // FixIt +// const CharSourceRange FunctionNameSourceRange = +// CharSourceRange::getCharRange( +// Node.getBeginLoc(), Node.getArg(0)->getBeginLoc()); + +// Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, +// "std::copy_n("); +// } + +void ReplaceMemcpyWithStdCopy::renameFunction(DiagnosticBuilder &Diag, + const CallExpr *MemcpyNode) {} + void ReplaceMemcpyWithStdCopy::reorderArgs(DiagnosticBuilder &Diag, const CallExpr *MemcpyNode) { - std::array<std::string, 3> arg; + 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); + 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 { + 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]); + Diag << FixItHint::CreateReplacement(GetSourceRange(0), Arg[1]); - arg[2] = arg[1] + " + ((" + arg[2] + ") / sizeof(*(" + arg[1] + ")))"; - Diag << FixItHint::CreateReplacement(getSourceRange(1), arg[2]); + Arg[2] = Arg[1] + " + ((" + Arg[2] + ") / sizeof(*(" + Arg[1] + ")))"; + Diag << FixItHint::CreateReplacement(GetSourceRange(1), Arg[2]); - Diag << FixItHint::CreateReplacement(getSourceRange(2), arg[0]); + 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 +} // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h index 0f262bf839af24..0f847a39f474d5 100644 --- a/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h +++ b/clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyWithStdCopy.h @@ -11,13 +11,10 @@ #include "../ClangTidyCheck.h" #include "../utils/IncludeInserter.h" -#include <memory> -#include <string> -#include <vector> +#include "clang/AST/ASTTypeTraits.h" +#include "clang/Basic/LangOptions.h" -namespace clang { -namespace tidy { -namespace modernize { +namespace clang::tidy::modernize { // Replace the C memcpy function with std::copy class ReplaceMemcpyWithStdCopy : public ClangTidyCheck { @@ -27,23 +24,31 @@ class ReplaceMemcpyWithStdCopy : public ClangTidyCheck { 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; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } + + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: + void handleMemcpy(); + void handleMemset(); + void renameFunction(DiagnosticBuilder &Diag, const CallExpr *MemcpyNode); void reorderArgs(DiagnosticBuilder &Diag, const CallExpr *MemcpyNode); void insertHeader(DiagnosticBuilder &Diag, const CallExpr *MemcpyNode, SourceManager *const SM); + bool checkIsByteSequence(const ast_matchers::MatchFinder::MatchResult &Result, + std::string_view Prefix); private: - std::unique_ptr<utils::IncludeInserter> Inserter; - utils::IncludeInserter IncludeInserter; - const utils::IncludeSorter::IncludeStyle IncludeStyle; + utils::IncludeInserter Inserter; }; -} // namespace modernize -} // namespace tidy -} // namespace clang +} // namespace clang::tidy::modernize #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 2d14f5ec913239..cd256bb8074f88 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -217,8 +217,8 @@ Changes in existing checks member function calls too and to only expand macros starting with ``PRI`` and ``__PRI`` from ``<inttypes.h>`` in the format string. -- New :doc:`modernize-replace-memcpy-with-stdcopy - <clang-tidy/checks/modernize-replace-memcpy-by-stdcopy>` check. +- New :doc:`modernize-replace-memcpy-with-std-copy + <clang-tidy/checks/modernize-replace-memcpy-with-std-copy>` check. Replaces all occurrences of the C ``memcpy`` function by ``std::copy``. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index de8197f20b39a4..e923f43dd44a67 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -286,7 +286,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-memcpy-with-std-copy <modernize/replace-memcpy-with-std-copy>`, "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/replace-memcpy-with-stdcopy.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/replace-memcpy-with-stdcopy.rst new file mode 100644 index 00000000000000..8708fe3b6e665e --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/replace-memcpy-with-stdcopy.rst @@ -0,0 +1,47 @@ +.. title:: clang-tidy - modernize-replace-memcpy-with-std-copy + +modernize-replace-memcpy-with-std-copy +=================================== + +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`. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits