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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
