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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.
Herald added a subscriber: carlosgalvezp.


================
Comment at: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt:19
   ReplaceAutoPtrCheck.cpp
+  ReplaceMemcpyByStdCopy.cpp
   ReplaceRandomShuffleCheck.cpp
----------------
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.)


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:35
+                                           isExpansionInSystemHeader())),
+               isExpansionInMainFile())
+          .bind("memcpy_function");
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:70
+                                            const CallExpr *MemcpyNode) {
+  const CharSourceRange FunctionNameSourceRange = CharSourceRange::getCharRange(
+      MemcpyNode->getBeginLoc(), MemcpyNode->getArg(0)->getBeginLoc());
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:78
+                                         const CallExpr *MemcpyNode) {
+  std::array<std::string, 3> arg;
+
----------------
LLVM naming convention is `CapCamelCase` for variables.
See here and variables declared on lines 85, 86, 91, etc.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:91
+  // Create lambda that return SourceRange of an argument
+  auto getSourceRange = [MemcpyNode](uint8_t ArgCount) -> SourceRange {
+    return SourceRange(MemcpyNode->getArg(ArgCount)->getBeginLoc(),
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:99
+
+  arg[2] = arg[1] + " + ((" + arg[2] + ") / sizeof(*(" + arg[1] + ")))";
+  Diag << FixItHint::CreateReplacement(getSourceRange(1), arg[2]);
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:11
+#include "../utils/OptionsUtils.h"
+
+#include <array>
----------------
Eugene.Zelenko wrote:
> Please remove unnecessary empty line.
> Please remove unnecessary empty line.

Where is this in the LLVM coding style guide?


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.h:26
+  ReplaceMemcpyByStdCopy(StringRef Name, ClangTidyContext *Context);
+  virtual ~ReplaceMemcpyByStdCopy() {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
----------------
Eugene.Zelenko wrote:
> Should be override and = default. See modernize-use-override and modernize-use-equals-default.
> Should be override and = default

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


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-memcpy-by-stdcopy.rst:33
+
+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)``
----------------
Please use a line length in the `.rst` files that is consistent with the LLVM line length
for code files.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63324/new/

https://reviews.llvm.org/D63324