[clang-tidy] Add check 'modernize-use-algorithm'

[JDevlieghere]

Summary

This check emits a warning when memcpy or memset is used and suggest replacing it with a call to std::copy and std::fill respectively.

For the motivation see modernize-use-algorithm.rst

Taken from: https://llvm.org/bugs/show_bug.cgi?id=22209

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

May be modernize will be better place then misc?

[aaron.ballman]

Using std::copy opens up the possibility of type-aware optimizations which are not possible with memcpy.

To my knowledge, those type-aware optimizations are for transforming copies involving trivially-copyable types into calls to memcpy(). What other optimizations does this check enable?

[JDevlieghere]

In D22725#493940, @aaron.ballman wrote:

Using std::copy opens up the possibility of type-aware optimizations which are not possible with memcpy.

To my knowledge, those type-aware optimizations are for transforming copies involving trivially-copyable types into calls to memcpy(). What other optimizations does this check enable?

I might be mistaken, but I understood that a call to std::copy (whether or not specialized for certain types) might get inlined, while a call to memcpy is typically not.

[Prazek]

Thanks for the contribution. Is it your first check?

Some main issues:

1. I think it would be much better to move this check to modernize module. I think the name 'modernize-use-copy' would follow the convention, or just

'modernize-replace-memcpy' also works, maybe it is even better. Your choice.
Use rename_check.py to rename the check.

2. Please add include fixer that will include <algorithm> if it is not yet included, so the code will compile after changes.

Look at DeprecatedHeadersCheck.cpp or PassByValueCheck ( or grep for registerPPCallbacks)

3. The extra parens are not ideal thing. Can you describe (with examples) in which situations it would not compile/ something bad would happen if you wouldn't put the parens? I think you could check it in matchers and then apply parens or not.

4. Have you run the check on LLVM code base? It uses std::copy in many places. If after running with -fix all the memcpys will be gone (check if if finds anything after second run) and the code will compile and tests will not fail then it means that your check fully works!

clang-tidy/misc/ReplaceMemcpyCheck.cpp

25 ↗

(On Diff #65221)

add if here to discard the non c++ calls. (the same as you see in most of the checks)

26 ↗

(On Diff #65221)

you can check argument count here.

40–41 ↗

(On Diff #65221)

so this can be checked in matcher

55 ↗

(On Diff #65221)

don't make replacement if it is in macro (use .isMacroID on the location).

Also add tests with macros.

clang-tidy/misc/ReplaceMemcpyCheck.h

30 ↗

(On Diff #65221)

If it is static, then move it to .cpp file.

You could also remove static and arguments that you have access from class and move it to private.

Also I think the function name convention is lowerCamelCase

test/clang-tidy/misc-replace-memcpy.cpp

3–5 ↗

(On Diff #65221)

don't include the stl, because it will make the test undebugable (the ast is too big)

Mock the functions that you need - std::copy and std::memcpy. you don't have to give definition, but just make sure the declaration of the functions are the same.

Also check if check will add <algorithm> here.

[aaron.ballman]

In D22725#493942, @JDevlieghere wrote:

In D22725#493940, @aaron.ballman wrote:

Using std::copy opens up the possibility of type-aware optimizations which are not possible with memcpy.

To my knowledge, those type-aware optimizations are for transforming copies involving trivially-copyable types into calls to memcpy(). What other optimizations does this check enable?

I might be mistaken, but I understood that a call to std::copy (whether or not specialized for certain types) might get inlined, while a call to memcpy is typically not.

The optimizer can be quite smart about memcpy(). For instance: https://godbolt.org/g/INfHWE

If we know of specific cases where std::memcpy() is used, but std::copy() will provide better performance metrics, that would be really interesting to know.

[Prazek]

In D22725#493942, @JDevlieghere wrote:

In D22725#493940, @aaron.ballman wrote:

Using std::copy opens up the possibility of type-aware optimizations which are not possible with memcpy.

To my knowledge, those type-aware optimizations are for transforming copies involving trivially-copyable types into calls to memcpy(). What other optimizations does this check enable?

I might be mistaken, but I understood that a call to std::copy (whether or not specialized for certain types) might get inlined, while a call to memcpy is typically not.

So if it is something trivially copyable (just copying bytes) then std::copy would probably call memcpy inside - so it won't be optimization in our case (even if it will get inlined).

But it will definitelly make code cleaner and won't be slower, which would be good to mention in docs.

[JDevlieghere]

• Added new check to release notes
• Renamed to modernize-use-copy
• Addressed comments from Prazek

[JDevlieghere]

In D22725#493947, @Prazek wrote:

Thanks for the contribution. Is it your first check?

Yes, it is! :-)

Some main issues:

1. I think it would be much better to move this check to modernize module. I think the name 'modernize-use-copy' would follow the convention, or just

'modernize-replace-memcpy' also works, maybe it is even better. Your choice.
Use rename_check.py to rename the check.

Agreed, I moved and renamed the check.

2. Please add include fixer that will include <algorithm> if it is not yet included, so the code will compile after changes.

Look at DeprecatedHeadersCheck.cpp or PassByValueCheck ( or grep for registerPPCallbacks)

Done

3. The extra parens are not ideal thing. Can you describe (with examples) in which situations it would not compile/ something bad would happen if you wouldn't put the parens? I think you could check it in matchers and then apply parens or not.

I have included an example in the documentation with the ternary conditional operator. I agree that it's not ideal, but I tried to be defensive. Basically anything with lower precedence than the + operator might cause an issue, and this is quite a lot...

4. Have you run the check on LLVM code base? It uses std::copy in many places. If after running with -fix all the memcpys will be gone (check if if finds anything after second run) and the code will compile and tests will not fail then it means that your check fully works!

Doing this as we speak!

[Eugene.Zelenko]

docs/ReleaseNotes.rst

70

Please highlight memcpy and std::copy with `` as it done in documentation.

[Prazek]

clang-tidy/modernize/UseCopyCheck.cpp

49 ↗

(On Diff #65246)

there is actually llvm::make_shared somewhere

59–61 ↗

(On Diff #65246)

What you should do is you should print out the warning, but not make the replacement for macro (because you can't get the exact location)

So just move auto Diag = ... above this line.

Also finish comments with coma.

test/clang-tidy/modernize-use-copy.cpp

17 ↗

(On Diff #65246)

This won't be checked. The long checking comments are ok in tests.

34 ↗

(On Diff #65246)

change it to MEMCPY, and maybe remove the last argument (so it will be set to sizeof dest), because this macro doesn't make much sense right now.

37 ↗

(On Diff #65246)

so ter should be a warning here.

[Prazek]

In D22725#493970, @JDevlieghere wrote:

In D22725#493947, @Prazek wrote:

Thanks for the contribution. Is it your first check?

Yes, it is! :-)

Cool! hope to see some other cool checks. You could probably use pretty much the same code, and make modernize-use-fill for replacing memset and
modernize-use-move to change memmove -> move. The other thing that I can think of, is also to detect when you could use std::copy_n istead.

Maybe the right way would be to have check called 'modernize-use-sequence-algorithm' or just 'modernize-use-algorithm' that would basically do all those stuff. It would be good to not introduce 5 new check that pretty much do the same thing and also the user would want to have them all or none.
You can look how I did it with modernize-use-make-shared, it would be pretty much the same.

I have included an example in the documentation with the ternary conditional operator. I agree that it's not ideal, but I tried to be defensive. Basically anything with lower precedence than the + operator might cause an issue, and this is quite a lot...

So It should not be very hard. You would just have to check if the second or third argument to memcpy is one of [list here] of the things that
would make it bad. And you can probably write easy matcher for this.
I understand that it probably will take the double of time that you have spend on this check, but this will actually is the thing here - you want your check to produce the fixes that makes sense. When you will run it on llvm code base, I guess you will see that over 90% of the cases didn't require coma. I understand that you want to make it accurate, but the fix should not also intoruce some code that is not tidy enough.
So I guess either fix it, or wait what Alex will say about it. It might make it to upstream like this, but I hope you would fix it in some time :)

Anyway good job, hope to see some more checks from you!

4. Have you run the check on LLVM code base? It uses std::copy in many places. If after running with -fix all the memcpys will be gone (check if if finds anything after second run) and the code will compile and tests will not fail then it means that your check fully works!

Doing this as we speak!

When you will be finished just post a diff of changes after running it on llvm on phabricator and add me as subscriber :)

clang-tidy/modernize/UseCopyCheck.cpp

38 ↗

(On Diff #65246)

change memcpy to ::std::memcpy, and also make a test with some memcpy that is outside of std namespace.
You don't want to change someone else code because it uses the same name :)

[JDevlieghere]

• Extended check to replace memmove with std::move and memset with std::fill
• Renamed check to modernize-use-algorithm (I'd be equally fine with moving it to misc again)
• Added more documentation to better describe the goal of this check
• Fixed macro test and added cases for memmove and memset

[JDevlieghere]

In D22725#494074, @Prazek wrote:

Maybe the right way would be to have check called 'modernize-use-sequence-algorithm' or just 'modernize-use-algorithm' that would basically do all those stuff. It would be good to not introduce 5 new check that pretty much do the same thing and also the user would want to have them all or none.
You can look how I did it with modernize-use-make-shared, it would be pretty much the same.

I kept everything in modernize-use-algorithm as I agree this isn't worth 3 different checks.

So It should not be very hard. You would just have to check if the second or third argument to memcpy is one of [list here] of the things that
would make it bad. And you can probably write easy matcher for this.
I understand that it probably will take the double of time that you have spend on this check, but this will actually is the thing here - you want your check to produce the fixes that makes sense. When you will run it on llvm code base, I guess you will see that over 90% of the cases didn't require coma. I understand that you want to make it accurate, but the fix should not also intoruce some code that is not tidy enough.
So I guess either fix it, or wait what Alex will say about it. It might make it to upstream like this, but I hope you would fix it in some time :)

You are right. I didn't completely finish the first run on LLVM, but none of the occurrences it had found so far would have caused an issue. I guess that means that omitting the extra parens is a sensible default for now. I will look into excluding problematic situations once I am sure this check is fully functional.

Anyway good job, hope to see some more checks from you!

Thanks, I really appreciate the feedback from everyone! :)

4. Have you run the check on LLVM code base? It uses std::copy in many places. If after running with -fix all the memcpys will be gone (check if if finds anything after second run) and the code will compile and tests will not fail then it means that your check fully works!

Doing this as we speak!

When you will be finished just post a diff of changes after running it on llvm on phabricator and add me as subscriber :)

It's running again with the changes from the last revision included. It is taking a very long time though but I guess that's my fault for not building in release mode. I'll leave it running for now as I don't want to start over again.

[Prazek]

hmm It seems that I mislead you, I suck at C api - memmove source and destination can overlap, but std::move can't. So I guess you have to remove the memmove support. Sorry.

[JDevlieghere]

In D22725#494167, @Prazek wrote:

hmm It seems that I mislead you, I suck at C api - memmove source and destination can overlap, but std::move can't. So I guess you have to remove the memmove support. Sorry.

No problem, I wasn't aware of the issue either, so thanks for letting me know!

While running my check on the LLVM repo I ran into another issue: can't do pointer arithmetic when the arguments to memcpy are void pointers. I will look into updating my matchers to exclude this particular case.

[Prazek]

In D22725#494168, @JDevlieghere wrote:

In D22725#494167, @Prazek wrote:

hmm It seems that I mislead you, I suck at C api - memmove source and destination can overlap, but std::move can't. So I guess you have to remove the memmove support. Sorry.

No problem, I wasn't aware of the issue either, so thanks for letting me know!

While running my check on the LLVM repo I ran into another issue: can't do pointer arithmetic when the arguments to memcpy are void pointers. I will look into updating my matchers to exclude this particular case.

Yep, developing clang-tidy checks is mostly about trying to find out all the cases, and then finding that there are 3x more corner cases after running it on LLVM :)
btw I recommend compiling on Release with enabled assertions (-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON). Usually you don't need debug info while developing clang-tidy because it doesn't crash that often and Release is at least 4x faster than RelWithDebInfo (and even more than Debug), so it actually make sense to recompile everything and run it than to wait for clang-tidy compiled with debug.

[etienneb]

In D22725#494167, @Prazek wrote:

hmm It seems that I mislead you, I suck at C api - memmove source and destination can overlap, but std::move can't. So I guess you have to remove the memmove support. Sorry.

On windows (MSVC CRT), both function are the same : memmove.

[etienneb]

clang-tidy/modernize/UseAlgorithmCheck.cpp

74

It's more efficient to use hasAnyName and merge the three following matcher.
That way, you won't need the function 'getCallExpr' to retrieve the CallExpr.

81

allOf is not needed.

107

Instead of DirectCallee, you could add a "bind" to the Matcher : functionDecl(...).bind("callee")

108

What is the name is not in the map?

[Prazek]

In D22725#495329, @etienneb wrote:

In D22725#494167, @Prazek wrote:

hmm It seems that I mislead you, I suck at C api - memmove source and destination can overlap, but std::move can't. So I guess you have to remove the memmove support. Sorry.

On windows (MSVC CRT), both function are the same : memmove.

It is possible that the call to move with some trivialy copyable object would end up calling memmove, even if the call is wrong.

quote from http://en.cppreference.com/w/cpp/algorithm/move

d_first - the beginning of the destination range. If d_first is within [first, last), std::move_backward must be used instead of std::move.

But this is only the special ability of memmove, so we should not introduce stl-implementation dependent bugs

[JDevlieghere]

Thanks for the reviews everyone!

I'm currently still stuck on an issue I discovered when processing LLVM. I asked for help on the mailing list [0] but maybe someone here can help. Basically the issue is that sometimes pointers are passed to memcpy for which the types are not assignable. That's okay for memcpy because it takes its arguments as void pointers, but of course this causes std::copy to complain.

I want my pass to warn the user about this issue but skip the substitution, but I don't know how to check for this case...

[0] http://lists.llvm.org/pipermail/cfe-dev/2016-July/050105.html

[JDevlieghere]

• Addressed concerns raised by Etienne Bergeron
• Show warning when using void pointers but don't replace
• Don't make replacement when destination is const
• Don't make replacement when types are not compatible

[Prazek]

I see you solved the void and conmpatible types problems. Excellent!

Can you post a patch with changes after running LLVM? I would wait for Alex or Aaron to accept it.

clang-tidy/modernize/UseAlgorithmCheck.cpp

59–61

Try just initializer list instead of creating vector here.
Just
for (const auto &keyValue : {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}

You just have to specify the type of the first argument and it will try to convert each next to it.

99–100

this should not happen. You can change it to assert

140

Would it be the same to check here for the first argument instead of second?
So then you could take this if before the if(MatchedName == "memset")

docs/clang-tidy/checks/modernize-use-algorithm.rst

38

You should add 2 notes here

1. The check runs just with C++.
2. There are some rare cases that require adding parens (and put example), so the check could right now generate wrong output. (which is something that I would not require to have, but it is good to leave not about it in documentation)

Also leave Fixit comment in the check.

[JDevlieghere]

Addressed comments from Piotr Padlewski

LLVM compiles with the latest version of this check, however some tests are failing. I'll investigate the cause of this and update this check if it can be prevented.

[Prazek]

In D22725#502374, @JDevlieghere wrote:

Addressed comments from Piotr Padlewski

LLVM compiles with the latest version of this check, however some tests are failing. I'll investigate the cause of this and update this check if it can be prevented.

That's weird. I wonder what happen, because the transformations looks legit.

[aaron.ballman]

clang-tidy/modernize/UseAlgorithmCheck.cpp

26

I'd like to see some test cases involving attributed types, typedefs to pointers and non-pointers, and multidimensional arrays. For instance, I'm curious how well this handles typedef int some_type; typedef some_type * const * volatile *foo_ptr; or typedef void (__cdecl fn_ptr)(int);

50–51

Please use Twine for this sort of thing.

66

Missing full stop.

94

Please do not use auto here as the type is not spelled out in the RHS (same elsewhere).

97

You should ensure that the callee is actually the function we care about. Consider:

namespace awful {
void memcpy(int, int, int);
}
using namespace awful;

Such a use of memcpy() would trigger your check and create an invalid replacement, I think (and there should be a test for this).

98

Please add some "help text" to the assert in case it ever triggers. e.g., && "Replacements list does not match list of registered matcher names" or some such.

102

Diagnostics should start with a lowercase letter.

More importantly, this warning doesn't actually describe a problem to the user. Given the fact that this only should be triggered on uses where memcpy and std::copy are interchangeable, it's not really a *warning* at all (due to the interchangeability) as there is nothing dangerous about the original code that the suggestion will fix. @alexfh, what do you think of the idea of this being a Note rather than a Warning? I know it's unorthodox, but literally every instance of this diagnostic can be ignored or replaced and there should be no semantic effect on the code either way. Most other checks in modernize have more obvious benefits to modifying the code and so Warning is reasonably appropriate.

If this text remains a warning, it should describe what is dangerous with the code, not simply how to transform the code. Perhaps "'memcpy' reduces type-safety, consider using 'std::copy' instead" or something along those lines?

108–109

What about parens? e.g., memcpy((foo + 12), (bar), (a + b - 10));

119

Same for function pointer types.

138

Missing a colon at the end of this line.

clang-tidy/modernize/UseAlgorithmCheck.h

2

Line length does not match the closing ASCII art line length.

21

This is no longer accurate, and is also missing the full stop.

docs/clang-tidy/checks/modernize-use-algorithm.rst

7

Spurious comma.

8

Comma before "respectively".

9

Comma before "and" because Oxford commas are the best commas.

11–12

Remove the "Furthermore," as this is a continuation of the "type-aware" statement above?

test/clang-tidy/modernize-use-algorithm.cpp

3

Remove blank line.

Missing tests for unqualified uses of calling memcpy() and memset().

27

The asterisk binds to the identifier -- should run clang-format over the file.

[JDevlieghere]

Addresses comments from Aaron Ballman

@aaron.ballman Thanks for the thorough review! Can you check whether the tests I added address your concerns? Could you also elaborate on the case with the C-function pointer? Unless I explicitly cast it to void* the compiler rejects will reject it as an argument to memcpy. Am I missing a case where this could go wrong? I still added it to the pointer arithmetic check though, just to be sure.

[Prazek]

In D22725#505020, @JDevlieghere wrote:

Addresses comments from Aaron Ballman

@aaron.ballman Thanks for the thorough review! Can you check whether the tests I added address your concerns? Could you also elaborate on the case with the C-function pointer? Unless I explicitly cast it to void* the compiler rejects will reject it as an argument to memcpy. Am I missing a case where this could go wrong? I still added it to the pointer arithmetic check though, just to be sure.

Did you manage to see what was wrong in the transformation that you did on LLVM?

I have one idea, which should be fixed. Memset takes char as an argument, so if you would fill the array of ints with memset(arr, 0, sizeof(arr)), then it will set all
the elements to zero. But if you would do it with with set say 1, you will end up having

int val = 1 | (1 << 8) | (1 << 16) | (1 << 24)

for each elemnt of the array.

I don't think tho that any LLVM code depend od this, you usually set everything to zero, or to another value if you operate on char type.

So allow transformation and diagnostic if the argument equals 0, or when the type is char.

[aaron.ballman]

In D22725#505020, @JDevlieghere wrote:

Addresses comments from Aaron Ballman

@aaron.ballman Thanks for the thorough review! Can you check whether the tests I added address your concerns? Could you also elaborate on the case with the C-function pointer? Unless I explicitly cast it to void* the compiler rejects will reject it as an argument to memcpy. Am I missing a case where this could go wrong? I still added it to the pointer arithmetic check though, just to be sure.

The tests added mostly cover them -- I elaborated on the function pointer case, which I don't *think* will go wrong with this check, but should have a pathological test case for just to be sure.

clang-tidy/modernize/UseAlgorithmCheck.cpp

27

Consider a pathological case like:

#include <string.h>

void f();

int main() {
  typedef void (__cdecl *fp)();
  
  fp f1 = f;
  fp f2;
  memcpy(&f2, &f1, sizeof(fp)); // transforms into std::copy(&f1, &f1, &f2); ?
}

The calling convention is important in the test because getStrippedType() isn't looking through attributed types, which I *think* that declaration would rely on. I don't think it will cause problems in this case, but the test will ensure it.

54–55

A more idiomatic way to write this is: return (Twine(Function) + "(" + Arg0...<etc>).str();

107

Instead of manually composing the diagnostic, it should use placeholders. e.g.,

diag(Loc, "%0 reduces type-safety, consider using '%1' instead) << Callee << ReplacedName;

(This makes it easier if we ever decide to localize our diagnostics.) Note that there's no quoting required around %0 because we're passing in a NamedDecl argument and the diagnostics engine understands how to display that.

[JDevlieghere]

• Added function pointer test case
• Used placeholders for diagnostics

I extended the matchers to include ::memcpy and ::memset as well because the check otherwise does not work on my mac because the cstring header that ships with Xcode is implemented as shown below.

_LIBCPP_BEGIN_NAMESPACE_STD
using ::size_t;
using ::memcpy;
using ::memset;
...
_LIBCPP_END_NAMESPACE_STD

This is annoying as I have no way to discriminate functions that have the same signature. Unless there's a better solution, this seems like a reasonable trade-off to me. Of course I'm open to suggestions!

In D22725#505739, @aaron.ballman wrote:

The tests added mostly cover them -- I elaborated on the function pointer case, which I don't *think* will go wrong with this check, but should have a pathological test case for just to be sure.

I added the test case. The call is indeed replaced, which I guess is fine?

In D22725#505476, @Prazek wrote:

Did you manage to see what was wrong in the transformation that you did on LLVM?

Not yet, thought it seemed to be related to std::copy rather than std::fill. I'm still trying to pinpoint the culprit but it's extremely time consuming as I have to recompile LLVM completely in order to run the tests...

[aaron.ballman]

In D22725#506058, @JDevlieghere wrote:

• Added function pointer test case
• Used placeholders for diagnostics

I extended the matchers to include ::memcpy and ::memset as well because the check otherwise does not work on my mac because the cstring header that ships with Xcode is implemented as shown below.

_LIBCPP_BEGIN_NAMESPACE_STD
using ::size_t;
using ::memcpy;
using ::memset;
...
_LIBCPP_END_NAMESPACE_STD

You should add a similar approach as a test case to ensure we're covering this usage.

This is annoying as I have no way to discriminate functions that have the same signature. Unless there's a better solution, this seems like a reasonable trade-off to me. Of course I'm open to suggestions!

Given that you also have to handle the case where C++ code includes string.h (rather than cstring) to call memcpy, I think this is the only approach you can really take. Either you are getting a global declaration or one in the std namespace, and both should be handled when the signatures are correct.

In D22725#505739, @aaron.ballman wrote:

The tests added mostly cover them -- I elaborated on the function pointer case, which I don't *think* will go wrong with this check, but should have a pathological test case for just to be sure.

I added the test case. The call is indeed replaced, which I guess is fine?

Yes, that's the behavior I would expect.

In D22725#505476, @Prazek wrote:

Did you manage to see what was wrong in the transformation that you did on LLVM?

Not yet, thought it seemed to be related to std::copy rather than std::fill. I'm still trying to pinpoint the culprit but it's extremely time consuming as I have to recompile LLVM completely in order to run the tests...

[alexfh]

clang-tidy/modernize/ModernizeTidyModule.cpp

60

Entries should be sorted alphabetically.

clang-tidy/modernize/UseAlgorithmCheck.cpp

51

I don't think this function pulls its weight. With just two callers and a trivial implementation, it seems that it's better to inline it.

60–62

I'm not sure this works on MSVC2013. Could someone try this before submitting?

87

For consistency, I'd prefer the early return style (if (!...) return;).

103

Notes are treated completely differently - each note has to be attached to a warning.

Clang-tidy can only deal with two severity levels of diagnostics: "warning" and "error", but it's better to let them be controlled by the user via -warnings-as-errors= command-line option or the WarningsAsErrors configuration file option.

104

This should be a StringRef instead. No need to allocate a string.

132

Should clang::tooling::fixit::getText be used instead?

clang-tidy/modernize/UseAlgorithmCheck.h

21

Please enclose inline code snippets in backquotes.

[aaron.ballman]

clang-tidy/modernize/UseAlgorithmCheck.cpp

60–62

It does not work in MSVC 2013.

103

Drat. I am not keen on this being a warning (let alone an error) because it really doesn't warn the user against anything bad (the type safety argument is tenuous at best), and it's arguable whether this actually modernizes code. Do you think there's sufficient utility to this check to warrant it being default-enabled as part of the modernize suite? For instance, we have 368 instances of memcpy() and 171 instances of std::copy() in the LLVM code base, which is an example of a very modern C++ code base. I'm not opposed to the check, just worried that it will drown out diagnostics that have more impact to the user.

[alexfh]

clang-tidy/modernize/UseAlgorithmCheck.cpp

60–62

Thanks for verifying. Then it should be just

Replacements["memcpy"] = "std::copy";
Replacements["memset"] = "std::fill";

Maybe even remove the map completely and just hardcode the two options in UseAlgorithmCheck::check. Or are we expecting other similar functions to be added to the list? (I guess, unlikely)

103

It's a terminology issue, I think. Clang-tidy uses clang "warnings" for all kinds of warnings, issues, suggestions or recommendations it reports. If we want to extend the range while remaining in the clang diagnostic engine bounds, we could use Remark for some diagnostics, but I think, the mapping would still need to be configurable by the user. Alternatively, we could introduce some kind of a more granular integer-based severity level. However, I don't have an immediate use case for either of these, so I'd leave design to someone, who actually needs this and can explain the motivation.

[JDevlieghere]

Fixes issues raised by Alexander and Aaron

[JDevlieghere]

Removed anonymous namespaces in test file. I was playing around with it but forgot to remove it before making my last diff.

[alexfh]

clang-tidy/modernize/UseAlgorithmCheck.cpp

38

Please inline this function as well. It's only used once and the implementation + the comment that is already in the call site are more than enough to explain the intent.

42

I'd probably inline this function as well. Its name is not particularly clear anyway (could be better with 'doesNotSupportPointerArithmetic` or similar, but I think, inlining it is even better).

49

Actually, I don't think we need a map of exactly two entries, since the order of arguments still has to be hardcoded. Let's just do a couple of ifs or a StringSwitch in the check() method.

142

No braces around single-statement if bodies, please.

docs/clang-tidy/checks/modernize-use-algorithm.rst

52

I think, we need to fix this right away instead of documenting this bug. Two things here:

1. problems related to the operator precedence can be fixed by adding parentheses, when needed (see how a similar issue was solved in SimplifyBooleanExprCheck; here we can make a needsParensBeforePlus function similar to needsParensAfterUnaryNegation).
2. the check should avoid replacements that duplicate an expression with side effects that can result from increment/decrement operators, assignments and some function calls. Function calls is the most difficult part here, since not all function calls have side effects (or are expensive). One approach here could be to add a const auto & variable to keep the value of the argument we need to repeat, if it's anything more complex than just a DeclRefExpr.

[JDevlieghere]

Still working on comment #2 from Alex but wanted to update my diff since it's been a while and I haven't gotten around to looking into it further. So no need to review yet.

[malcolm.parsons]

This check looks like it implements some of CppCoreGuidelines Bounds.4

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#bounds4-dont-use-standard-library-functions-and-types-that-are-not-bounds-checked

[alexfh]

Removing from my dashboard. Ping me once you have a new patch.

[JDevlieghere]

I'm abandoning this revision because I think this check is getting overly complex. There's still the problem of supporting arguments that can have side effects, and then there's also the unaddressed issue of code possibly using the void* return type of these two functions. Any solution to this would require rather inelegant and complex code that is, in my opinion, simply not warranted by the added value of this check.

clang-tidy/modernize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyModernizeModule
   AvoidBindCheck.cpp
   DeprecatedHeadersCheck.cpp
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp
   MakeSmartPtrCheck.cpp
   MakeSharedCheck.cpp
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
   ShrinkToFitCheck.cpp
+  UseAlgorithmCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultCheck.cpp
   UseEmplaceCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseUsingCheck.cpp
 

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
 #include "ShrinkToFitCheck.h"
+#include "UseAlgorithmCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseUsingCheck.h"
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<RawStringLiteralCheck>(
         "modernize-raw-string-literal");
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
+    CheckFactories.registerCheck<UseAlgorithmCheck>("modernize-use-algorithm");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");
     CheckFactories.registerCheck<UseBoolLiteralsCheck>(
         "modernize-use-bool-literals");
     CheckFactories.registerCheck<UseDefaultCheck>("modernize-use-default");
     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;

clang-tidy/modernize/UseAlgorithmCheck.h

+//===--- UseAlgorithmCheck.h - clang-tidy-------------------------- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_ALGORITHM_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_ALGORITHM_H
+
+#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replaces `memcpy` and `memset` with `std::copy` and `std::fill`,
+/// respectively.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-algorithm.html
+class UseAlgorithmCheck : public ClangTidyCheck {
+public:
+  UseAlgorithmCheck(StringRef Name, ClangTidyContext *Context);
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::unique_ptr<utils::IncludeInserter> Inserter;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_ALGORITHM_H

clang-tidy/modernize/UseAlgorithmCheck.cpp

+//===--- UseAlgorithmCheck.cpp - clang-tidy--------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseAlgorithmCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/FixIt.h"
+
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+static QualType getStrippedType(QualType T) {
+  while (const auto *PtrType = T->getAs<PointerType>())
+    T = PtrType->getPointeeType();
+
+  if (const auto *ArrType = dyn_cast<ArrayType>(T))
+    T = ArrType->getElementType();
+
+  return T.getCanonicalType().getUnqualifiedType();
+}
+
+static bool doesNotSupportPointerArithmetic(QualType T) {
+  return T->isVoidPointerType() || T->isFunctionPointerType();
+}
+
+static bool needsParensAroundExpressions(const Expr *E) {
+  E = E->IgnoreImpCasts();
+
+  if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
+    return true;
+
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
+    return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
+           Op->getOperator() != OO_Subscript;
+
+  return false;
+}
+
+UseAlgorithmCheck::UseAlgorithmCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+          Options.get("IncludeStyle", "llvm"))) {}
+
+void UseAlgorithmCheck::registerMatchers(MatchFinder *Finder) {
+  // Only register the matchers for C++.
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // Match calls to memcpy or memset with exactly 3 arguments.
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasAnyName("::std::memcpy", "::std::memset",
+                                              "::memcpy", "::memset"))
+                          .bind("callee")),
+               argumentCountIs(3))
+          .bind("expr"),
+      this);
+}
+
+void UseAlgorithmCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  // Only register the preprocessor callbacks for C++; the functionality
+  // currently does not provide any benefit to other languages, despite being
+  // benign.
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Inserter = llvm::make_unique<utils::IncludeInserter>(
+      Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle);
+  Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void UseAlgorithmCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("expr");
+  const auto *Callee = Result.Nodes.getNodeAs<FunctionDecl>("callee");
+
+  const StringRef MatchedName = Callee->getName();
+
+  // Check if matched name is in map of replacements.
+  StringRef ReplacedName;
+  if (MatchedName == "memcpy") {
+    ReplacedName = "std::copy";
+  } else if (MatchedName == "memset") {
+    ReplacedName = "std::fill";
+  } else {
+    return;
+  }
+
+  const SourceLocation Loc = MatchedExpr->getExprLoc();
+  auto Diag = diag(Loc, "%0 reduces type-safety, consider using '%1' instead")
+              << Callee << ReplacedName;
+
+  // Don't make replacements in macro.
+  if (Loc.isMacroID())
+    return;
+
+  const QualType Arg0Type = MatchedExpr->getArg(0)->IgnoreImpCasts()->getType();
+  const QualType Arg1Type = MatchedExpr->getArg(1)->IgnoreImpCasts()->getType();
+
+  // Don't make replacements when destination is const.
+  if (Arg0Type.isConstQualified())
+    return;
+
+  // Don't make replacements when types are not compatible.
+  if (getStrippedType(Arg0Type) != getStrippedType(Arg1Type))
+    return;
+
+  // Don't make replacements when types do not support pointer arithmetic.
+  if (doesNotSupportPointerArithmetic(Arg0Type) ||
+      doesNotSupportPointerArithmetic(Arg1Type))
+    return;
+
+  StringRef Arg0Text =
+      tooling::fixit::getText(*MatchedExpr->getArg(0), *Result.Context);
+  if (needsParensAroundExpressions(MatchedExpr->getArg(0)))
+    Arg0Text = ("(" + Arg0Text + ")").str();
+
+  StringRef Arg1Text =
+      tooling::fixit::getText(*MatchedExpr->getArg(1), *Result.Context);
+  if (needsParensAroundExpressions(MatchedExpr->getArg(1)))
+    Arg1Text = ("(" + Arg1Text + ")").str();
+
+  StringRef Arg2Text =
+      tooling::fixit::getText(*MatchedExpr->getArg(2), *Result.Context);
+  if (needsParensAroundExpressions(MatchedExpr->getArg(2)))
+    Arg2Text = ("(" + Arg2Text + ")").str();
+
+  std::string Insertion;
+  if (MatchedName == "memset") {
+    // Rearrangement of arguments for memset:
+    // (dest, ch, count) becomes (dest, dest + count, ch).
+    Insertion = (ReplacedName + "(" + Arg0Text + ", " + Arg0Text + " + " +
+                 Arg2Text + ", " + Arg1Text + ")")
+                    .str();
+  } else {
+    // Rearrangement of arguments for memcpy:
+    // (dest, src, count) becomes (src, src + count, dest).
+    Insertion = (ReplacedName + "(" + Arg1Text + ", " + Arg1Text + " + " +
+                 Arg2Text + ", " + Arg0Text + ")")
+                    .str();
+  }
+
+  Diag << FixItHint::CreateReplacement(MatchedExpr->getSourceRange(),
+                                       Insertion);
+
+  if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
+          Result.SourceManager->getFileID(Loc), "algorithm",
+          /*IsAngled=*/true))
+    Diag << *IncludeFixit;
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 - New `cppcoreguidelines-slicing
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ check
 
   Flags slicing of member variables or vtable.
 
 - New `cppcoreguidelines-special-member-functions
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html>`_ check
 
   Flags classes where some, but not all, special member functions are user-defined.
 
+- New `modernize-use-algorithm
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-algorithm.html>`_ check
+
+  Replaces calls to ``memcpy`` and ``memset`` with their respective algorithm
+  counterparts ``std::copy`` and ``std::fill``.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-loop-convert
    modernize-make-shared
    modernize-make-unique
    modernize-pass-by-value
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    modernize-shrink-to-fit
+   modernize-use-algorithm
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default
    modernize-use-emplace
    modernize-use-nullptr
    modernize-use-override
    modernize-use-using
    mpi-type-mismatch

docs/clang-tidy/checks/modernize-use-algorithm.rst

+.. title:: clang-tidy - modernize-use-algorithm
+
+modernize-use-algorithm
+=======================
+
+Replaces calls to ``std::memcpy`` and ``std::memset`` with ``std::copy`` and
+``std::fill``, respectively. The advantages of using these algorithm functions
+is that they are at least as efficient, more general, and type-aware.
+
+By using the algorithms the types remain intact as opposed to being discarded
+by the C-style functions. This allows the implementation to make use use of
+type information to further optimize. One example of such optimization is
+taking advantage of 64-bit alignment when copying an array of
+``std::uint64_t``.
+
+memcpy
+------
+
+.. code:: c++
+
+    std::memcpy(dest, source, sizeof dest);
+
+    // transforms to:
+
+    std::copy(source, source + sizeof dest, dest);
+
+memset
+------
+
+.. code:: c++
+
+    std::memset(dest, ch, count);
+
+    // transforms to:
+
+    std::fill(dest, dest + count, ch)
+
+Limitations
+-----------
+
+Because of the precedence of the additieve operator, it is possible that the
+resulting transformation changes the semantics of the original code. An example
+is given below.
+
+.. code:: c++
+
+    std::memcpy(dest, foo ? bar : baz, size);
+
+    // transforms to:
+
+    std::copy(foo ? bar : baz, foo ? baz : baz + size, dest);
+
+The meaning of the expression changes because the ternary conditional operator
+has a lower precendence than the additive operator.
+
+.. code:: c++
+
+    std::copy(foo ? bar : baz, foo ? baz : (baz + size), dest);
+
+    // but should have been:
+
+    std::copy(foo ? bar : baz, (foo ? baz : baz) + size, dest);

test/clang-tidy/modernize-use-algorithm.cpp

+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+// CHECK-FIXES: #include <algorithm>
+
+namespace std {
+typedef unsigned int size_t;
+void *memcpy(void *dest, const void *src, std::size_t count);
+void *memset(void *dest, int ch, std::size_t count);
+
+template <class InputIt, class OutputIt>
+OutputIt copy(InputIt first, InputIt last, OutputIt d_first);
+
+template <class InputIt, class OutputIt>
+OutputIt move(InputIt first, InputIt last, OutputIt d_first);
+
+template <class ForwardIt, class T>
+void fill(ForwardIt first, ForwardIt last, const T &value);
+}
+
+typedef unsigned int size_t;
+void *memcpy(void *dest, const void *source, size_t count);
+void *memset(void *dest, int ch, size_t count);
+
+namespace alternative_std {
+using ::memcpy;
+using ::memset;
+}
+
+namespace awful {
+void memcpy(int, int, int);
+}
+
+typedef unsigned int size_t;
+void *memcpy(void *dest, const void *source, size_t size);
+
+void a() {
+  char foo[] = "foo", bar[4];
+  std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(foo, foo + sizeof bar, bar);
+
+  alternative_std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(foo, foo + sizeof bar, bar);
+
+  void *baz = bar;
+  std::memcpy(baz, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+
+  memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(foo, foo + sizeof bar, bar);
+
+  std::copy(foo, foo + sizeof bar, bar);
+}
+
+void b() {
+  char foo[] = "foobar";
+  std::memset(foo, 'a', 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memset' reduces type-safety, consider using 'std::fill' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::fill(foo, foo + 3, 'a');
+
+  alternative_std::memset(foo, 'a', 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memset' reduces type-safety, consider using 'std::fill' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::fill(foo, foo + 3, 'a');
+
+  std::memset(foo, 1, 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memset' reduces type-safety, consider using 'std::fill' instead [modernize-use-algorithm]
+
+  std::fill(foo, foo + 2, 'a');
+}
+
+#define MEMCPY(dest, src) std::memcpy((dest), (src), sizeof(dest))
+void c() {
+  char foo[] = "foo", bar[3];
+  MEMCPY(bar, foo);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+}
+
+void d() {
+  typedef char foo_t;
+  typedef char bar_t;
+  foo_t foo[] = "foo";
+  bar_t bar[4];
+  std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(foo, foo + sizeof bar, bar);
+}
+
+void e() {
+  typedef const char *foo_t;
+  typedef const char *bar_t;
+  foo_t foo[] = {"foo", "bar"};
+  bar_t bar[2];
+  std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(foo, foo + sizeof bar, bar);
+}
+
+void f() {
+  typedef int some_type;
+  typedef some_type *const *volatile *foo_ptr;
+
+  typedef int *const some_other_type;
+  typedef some_other_type *volatile *bar_ptr;
+
+  foo_ptr foo[4];
+  bar_ptr bar[3];
+  std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(foo, foo + sizeof bar, bar);
+}
+
+void g() {
+  int foo[3][4] = {{0, 1, 2, 3}, {4, 5, 6, 7}, {8, 9, 10, 11}};
+  int bar[2][4];
+
+  std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(foo, foo + sizeof bar, bar);
+
+  int baz[3][3] = {{0, 1, 2}, {3, 4, 5}, {6, 7, 8}};
+  int qux[2][4];
+  std::memcpy(qux, baz, sizeof qux);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+}
+
+void h() {
+  int a = 1;
+  int b = 2;
+  int c = 3;
+
+  char foo[] = "foobar";
+  char bar[3];
+
+  std::memcpy((bar), (foo + b), (a + c - 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy((foo + b), (foo + b) + (a + c - 1), (bar));
+}
+
+void i() {
+  using namespace awful;
+  int foo = 1;
+  int bar = 2;
+  memcpy(bar, foo, sizeof bar);
+}
+
+void j() {
+  void f();
+  typedef void(__cdecl * fp)();
+
+  fp f1 = f;
+  fp f2;
+  std::memcpy(&f2, &f1, sizeof(fp));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' reduces type-safety, consider using 'std::copy' instead [modernize-use-algorithm]
+  // CHECK-FIXES: std::copy(&f1, &f1 + sizeof(fp), &f2);
+}