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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.
Herald added subscribers: wuzish, whisperity.

You need tests for this check. Please harden them against macro-interference as well. I think the transformation should happen as function call that return `Optional` and if any failure happens the diagnostic can still be emitted.



================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:27
+void ReplaceMemcpyByStdCopy::registerMatchers(MatchFinder *Finder) {
+  assert(Finder != nullptr);
+
----------------
this assertion is not necessary. `Finder` is always not-null.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:33
+  auto MemcpyMatcher =
+      callExpr(hasDeclaration(functionDecl(hasName("memcpy"),
+                                           isExpansionInSystemHeader())),
----------------
please match on `::memcpy` and `::std::memcpy`.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:53
+  const auto *MemcpyNode = Result.Nodes.getNodeAs<CallExpr>("memcpy_function");
+  assert(MemcpyNode != nullptr);
+
----------------
this assertion is not necessary, as you only match on one thing and that means it exists. (otherwise everything is broken already).


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

https://reviews.llvm.org/D63324