Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Malcolm Parsons via cfe-commits]

malcolm.parsons added a subscriber: malcolm.parsons.
malcolm.parsons added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:37
@@ +36,3 @@
+static bool areTypesCompatible(QualType Left, QualType Right) {
+  return getStrippedType(Left) == getStrippedType(Right);
+}

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.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:41
@@ +40,3 @@
+static bool noPointerArithmetic(QualType T) {
+  return T->isVoidPointerType() || T->isFunctionPointerType();
+}

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).


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:48
@@ +47,3 @@
+  Options.get("IncludeStyle", "llvm"))) {
+  Replacements["memcpy"] = "std::copy";
+  Replacements["memset"] = "std::fill";

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 `if`s or a 
`StringSwitch` in the `check()` method.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:141
@@ +140,3 @@
+  Result.SourceManager->getFileID(Loc), "algorithm",
+  /*IsAngled=*/true)) {
+Diag << *IncludeFixit;

No braces around single-statement `if` bodies, please.


Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:51
@@ +50,3 @@
+
+std::copy(foo ? bar : baz, foo ? baz : baz + size, dest);
+

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

2016-08-09 5:49 GMT-07:00 Aaron Ballman :
>
> I think this boils down to personal preference, which is why I'm
> concerned about the check. Either mechanism is correct, so this is
> purely a stylistic check in many regards.
>
> About warnings - well, if someone choose this check to be run, then he
> > probably wants to get warnings instead of notes.
>
> The problem is that people don't always choose this check to be run,
> they choose to run clang-tidy and this check is enabled by default. Or
> they choose to run modernize and this check is enabled by default.
>
> As with most checks. We can either be "perfect" and add only checks that
we consider very useful, or we can be open
and let people choose checks that they want from bigger set. After I used
clang-tidy first time, I know what checks are not something that I want, so
I just didn't use it.
I think we can work out on other solution. Maybe we should make some levels
of recomendation, so the user would use some option like --level=recommend
or something,
and we would have to somehow split the checks into groups. This would be
still problematic, but I know that it would be nice to have analyzer-alpha
checks into not recommend mode
(I don't know why they are used on default. They have so many bugs).

Anyway as you can see the matter of usefulness and very subjective. I can
argue that this is should belong to modernize, because I think that this is
modernization (changing code from
bugprone C ways to slightly better C++ ways), but of course everyone will
have own opinion.


Piotr

https://reviews.llvm.org/D22725
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Jonas Devlieghere via cfe-commits]

JDevlieghere updated this revision to Diff 67391.
JDevlieghere added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D22725

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseAlgorithmCheck.cpp
  clang-tidy/modernize/UseAlgorithmCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-algorithm.rst
  test/clang-tidy/modernize-use-algorithm.cpp

Index: test/clang-tidy/modernize-use-algorithm.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-algorithm.cpp
@@ -0,0 +1,156 @@
+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+// CHECK-FIXES: #include 
+
+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 
+OutputIt copy(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+OutputIt move(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+void fill(ForwardIt first, ForwardIt last, const T );
+}
+
+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 

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Jonas Devlieghere via cfe-commits]

JDevlieghere updated this revision to Diff 67390.
JDevlieghere marked 5 inline comments as done.
JDevlieghere added a comment.

Fixes issues raised by Alexander and Aaron


Repository:
  rL LLVM

https://reviews.llvm.org/D22725

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseAlgorithmCheck.cpp
  clang-tidy/modernize/UseAlgorithmCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-algorithm.rst
  test/clang-tidy/modernize-use-algorithm.cpp

Index: test/clang-tidy/modernize-use-algorithm.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-algorithm.cpp
@@ -0,0 +1,160 @@
+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+// CHECK-FIXES: #include 
+
+namespace {
+
+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 
+OutputIt copy(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+OutputIt move(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+void fill(ForwardIt first, ForwardIt last, const T );
+}
+
+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 

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Aaron Ballman via cfe-commits]

On Mon, Aug 8, 2016 at 11:36 PM, Piotr Padlewski
 wrote:
>
>
> 2016-08-08 8:33 GMT-07:00 Aaron Ballman :
>>
>> aaron.ballman added inline comments.
>>
>> 
>> Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:59-61
>> @@ +58,5 @@
>> +  IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
>> +  Options.get("IncludeStyle", "llvm"))) {
>> +
>> +  for (const auto  :
>> +   {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}})
>> {
>> 
>> alexfh wrote:
>> > I'm not sure this works on MSVC2013. Could someone try this before
>> > submitting?
>> It does not work in MSVC 2013.
>>
> What is the best way to fix it and still have a nice code?

As Alex pointed, out, just set the map values directly since there are only two.

>
>>
>> 
>> Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:102
>> @@ +101,3 @@
>> +  assert(it != Replacements.end() &&
>> + "Replacements list does not match list of registered matcher
>> names");
>> +  const std::string ReplacedName = it->second;
>> 
>> alexfh wrote:
>> > 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.
>> 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.
>>
> I consider memcpy and memset very bugprone. It is very easy to swap
> arguments by mistake or pass something with a different type etc. Also it is
> easier to understand fill than a memset, so it is easier for
> C++ programmers who see any of it first time to get it.
> If I would see someone using memcpy or memset in C++ code on the review,
> asking to change for the one from the algorithm would be my first comment.

I think this boils down to personal preference, which is why I'm
concerned about the check. Either mechanism is correct, so this is
purely a stylistic check in many regards.

> About warnings - well, if someone choose this check to be run, then he
> probably wants to get warnings instead of notes.

The problem is that people don't always choose this check to be run,
they choose to run clang-tidy and this check is enabled by default. Or
they choose to run modernize and this check is enabled by default.

> I understand your point,
> that maybe we should use something lighter for the "light" mistakes, but the
> user is the one that feels if something is bad or not, and he just choose
> the check that he thinks it is resonable to run.
> I would like to see some proposal how you exactly you would imagine good
> warnings, but I don't think we should discuss it in this review. We can
> change it after it will be in the trunk.

As Alex pointed out, this is immaterial. Warnings are the proper
mechanism for this.

> Also, could you respond in the phabricator? I am not sure how does it work,
> but sometimes responding in a mail works fine and there is a comment in
> phab, but many times it doesn't appear there.

I generally do, but often email is easier (especially depending on
which machine I respond from). If there are issues with phab not
picking up comments from emails, we should alert the Phab developers,
as I understand that to be a bug.

~Aaron

>
> Piotr
>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D22725
>>
>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

2016-08-08 8:33 GMT-07:00 Aaron Ballman :

> aaron.ballman added inline comments.
>
> 
> Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:59-61
> @@ +58,5 @@
> +  IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
> +  Options.get("IncludeStyle", "llvm"))) {
> +
> +  for (const auto  :
> +   {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}) {
> 
> alexfh wrote:
> > I'm not sure this works on MSVC2013. Could someone try this before
> submitting?
> It does not work in MSVC 2013.
>
> What is the best way to fix it and still have a nice code?


> 
> Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:102
> @@ +101,3 @@
> +  assert(it != Replacements.end() &&
> + "Replacements list does not match list of registered matcher
> names");
> +  const std::string ReplacedName = it->second;
> 
> alexfh wrote:
> > 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.
> 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.
>
> I consider memcpy and memset very bugprone. It is very easy to swap
arguments by mistake or pass something with a different type etc. Also it
is easier to understand fill than a memset, so it is easier for
C++ programmers who see any of it first time to get it.
If I would see someone using memcpy or memset in C++ code on the review,
asking to change for the one from the algorithm would be my first comment.

About warnings - well, if someone choose this check to be run, then he
probably wants to get warnings instead of notes. I understand your point,
that maybe we should use something lighter for the "light" mistakes, but
the user is the one that feels if something is bad or not, and he just
choose the check that he thinks it is resonable to run.
I would like to see some proposal how you exactly you would imagine good
warnings, but I don't think we should discuss it in this review. We can
change it after it will be in the trunk.

Also, could you respond in the phabricator? I am not sure how does it work,
but sometimes responding in a mail works fine and there is a comment in
phab, but many times it doesn't appear there.

Piotr


> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D22725
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:59-61
@@ +58,5 @@
+  IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+  Options.get("IncludeStyle", "llvm"))) {
+
+  for (const auto  :
+   {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}) {

aaron.ballman wrote:
> alexfh wrote:
> > I'm not sure this works on MSVC2013. Could someone try this before 
> > submitting?
> It does not work in MSVC 2013.
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)


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:102
@@ +101,3 @@
+  assert(it != Replacements.end() &&
+ "Replacements list does not match list of registered matcher names");
+  const std::string ReplacedName = it->second;

aaron.ballman wrote:
> alexfh wrote:
> > 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.
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.


Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:59
@@ -57,1 +58,3 @@
 "modernize-use-bool-literals");
+CheckFactories.registerCheck(
+"modernize-use-algorithm");

Entries should be sorted alphabetically.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:50
@@ +49,3 @@
+
+static std::string getReplacement(const StringRef Function,
+  const StringRef Arg0, const StringRef Arg1,

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.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:59-61
@@ +58,5 @@
+  IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+  Options.get("IncludeStyle", "llvm"))) {
+
+  for (const auto  :
+   {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}) {

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:86
@@ +85,3 @@
+  // benign.
+  if (getLangOpts().CPlusPlus) {
+Inserter = llvm::make_unique(

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:102
@@ +101,3 @@
+  assert(it != Replacements.end() &&
+ "Replacements list does not match list of registered matcher names");
+  const std::string ReplacedName = it->second;

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.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:103
@@ +102,3 @@
+ "Replacements list does not match list of registered matcher names");
+  const std::string ReplacedName = it->second;
+

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:131
@@ +130,3 @@
+
+  const StringRef Arg0Text = getText(MatchedExpr->getArg(0), SM, LangOptions);
+  const StringRef Arg1Text = getText(MatchedExpr->getArg(1), SM, LangOptions);

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.h:20
@@ +19,3 @@
+
+/// Replaces std::memcpy and std::memset with std::copy and std::fill,
+/// respectively.

Please enclose inline code snippets in backquotes.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/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 https://reviews.llvm.org/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 https://reviews.llvm.org/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...





Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Jonas Devlieghere via cfe-commits]

JDevlieghere updated this revision to Diff 66835.
JDevlieghere marked 9 inline comments as done.
JDevlieghere added a comment.

- 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 https://reviews.llvm.org/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 https://reviews.llvm.org/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...


Repository:
  rL LLVM

https://reviews.llvm.org/D22725

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseAlgorithmCheck.cpp
  clang-tidy/modernize/UseAlgorithmCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-algorithm.rst
  test/clang-tidy/modernize-use-algorithm.cpp

Index: test/clang-tidy/modernize-use-algorithm.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-algorithm.cpp
@@ -0,0 +1,139 @@
+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+// CHECK-FIXES: #include 
+
+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 
+OutputIt copy(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+OutputIt move(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+void fill(ForwardIt first, ForwardIt last, const T );
+}
+
+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);
+
+  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');
+
+  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 

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/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.



Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:26
@@ +25,3 @@
+static QualType getStrippedType(QualType T) {
+  while (const auto *PtrType = T->getAs())
+T = PtrType->getPointeeType();

Consider a pathological case like:
```
#include 

void f();

int main() {
  typedef void (__cdecl *fp)();
  
  fp f1 = f;
  fp f2;
  memcpy(, , sizeof(fp)); // transforms into std::copy(, , ); ?
}
```
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.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:53-54
@@ +52,4 @@
+  const StringRef Arg2) {
+  Twine Replacement = Function + "(" + Arg0 + ", " + Arg1 + ", " + Arg2 + ")";
+  return Replacement.str();
+}

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:106
@@ +105,3 @@
+  const SourceLocation Loc = MatchedExpr->getExprLoc();
+  auto Diag = diag(Loc, "'" + MatchedName.str() +
+"' reduces type-safety, consider using '" +

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Jonas Devlieghere via cfe-commits]

JDevlieghere updated this revision to Diff 66688.
JDevlieghere marked 21 inline comments as done.
JDevlieghere added a comment.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseAlgorithmCheck.cpp
  clang-tidy/modernize/UseAlgorithmCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-algorithm.rst
  test/clang-tidy/modernize-use-algorithm.cpp

Index: test/clang-tidy/modernize-use-algorithm.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-algorithm.cpp
@@ -0,0 +1,125 @@
+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+// CHECK-FIXES: #include 
+
+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 
+OutputIt copy(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+OutputIt move(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+void fill(ForwardIt first, ForwardIt last, const T );
+}
+
+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);
+
+  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);
+  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');
+
+  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: 

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Aaron Ballman via cfe-commits]

aaron.ballman requested changes to this revision.
This revision now requires changes to proceed.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:25
@@ +24,3 @@
+
+static QualType getStrippedType(QualType T) {
+  while (const auto *PtrType = T->getAs())

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);`


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:49-50
@@ +48,4 @@
+  const StringRef Arg2) {
+  return Function.str() + "(" + Arg0.str() + ", " + Arg1.str() + ", " +
+ Arg2.str() + ")";
+}

Please use Twine for this sort of thing.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:65
@@ +64,3 @@
+void UseAlgorithmCheck::registerMatchers(MatchFinder *Finder) {
+  // Only register the matchers for C++
+  if (!getLangOpts().CPlusPlus)

Missing full stop.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:93
@@ +92,3 @@
+
+  const auto MatchedName = Callee->getNameAsString();
+

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:96
@@ +95,3 @@
+  // Check if matched name is in map of replacements.
+  const auto it = Replacements.find(MatchedName);
+  assert(it != Replacements.end());

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).



Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:97
@@ +96,3 @@
+  const auto it = Replacements.find(MatchedName);
+  assert(it != Replacements.end());
+  const auto ReplacedName = it->second;

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.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:101
@@ +100,3 @@
+  const auto Loc = MatchedExpr->getExprLoc();
+  auto Diag = diag(Loc, "Use " + ReplacedName + " instead of " + MatchedName);
+

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?


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:107-108
@@ +106,4 @@
+
+  const auto Arg0Type = MatchedExpr->getArg(0)->IgnoreImpCasts()->getType();
+  const auto Arg1Type = MatchedExpr->getArg(1)->IgnoreImpCasts()->getType();
+

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:118
@@ +117,3 @@
+
+  // Cannot do arithmetic on void pointer.
+  if (getStrippedType(Arg0Type)->isVoidType() ||

Same for function pointer types.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:137
@@ +136,3 @@
+  } else {
+// Rearrangement of arguments for memcpy
+// (dest, src, count) becomes (src, src + count, dest).

Missing a colon at the end of this line.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.h:1
@@ +1,2 @@
+//===--- UseAlgorithmCheck.h - clang-tidy-*- C++ 
-*-===//
+//

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


Comment at: clang-tidy/modernize/UseAlgorithmCheck.h:20
@@ +19,3 @@
+
+/// Replaces memcpy with std::copy
+///

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


Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:6
@@ +5,3 @@
+
+Replaces 

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Jonas Devlieghere via cfe-commits]

JDevlieghere updated this revision to Diff 66350.
JDevlieghere added a comment.

Addressed comments from Piotr Padlewski


Repository:
  rL LLVM

https://reviews.llvm.org/D22725

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseAlgorithmCheck.cpp
  clang-tidy/modernize/UseAlgorithmCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-algorithm.rst
  test/clang-tidy/modernize-use-algorithm.cpp

Index: test/clang-tidy/modernize-use-algorithm.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-algorithm.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+
+// CHECK-FIXES: #include 
+
+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 
+OutputIt copy(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+OutputIt move(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+void fill(ForwardIt first, ForwardIt last, const T );
+}
+
+void a() {
+  char foo[] = "foo", bar[3];
+  std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use std::copy instead of memcpy [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: Use std::copy instead of memcpy [modernize-use-algorithm]
+
+  std::copy(foo, foo + sizeof bar, bar);
+}
+
+void b() {
+  char foo[] = "foobar";
+  std::memset(foo, 'a', 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use std::fill instead of memset [modernize-use-algorithm]
+  // CHECK-FIXES: std::fill(foo, foo + 3, 'a');
+
+  std::memset(foo, 1, 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use std::fill instead of memset [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: Use std::copy instead of memcpy [modernize-use-algorithm]
+}
Index: docs/clang-tidy/checks/modernize-use-algorithm.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-algorithm.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - modernize-use-algorithm
+
+modernize-use-algorithm
+===
+
+Replaces calls to ``memcpy`` and ``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.
+
+Furthermore, 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)
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -105,6 +105,7 @@
modernize-shrink-to-fit
modernize-use-auto
modernize-use-bool-literals
+   modernize-use-algorithm
modernize-use-default
modernize-use-emplace
modernize-use-nullptr
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -69,6 +69,12 @@
 
   Flags classes where some, but not all, special member functions are user-defined.
 
+- New `modernize-use-algorithm
+  `_ check
+
+  Replaces calls to ``memcpy`` and ``memset`` with their respective algorithm
+  counterparts ``std::copy`` and ``std::fill``.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseAlgorithmCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UseAlgorithmCheck.h
@@ -0,0 +1,42 @@
+//===--- 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 

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

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.



Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:58-60
@@ +57,5 @@
+
+  for (const auto  :
+   std::vector>(
+   {{"memcpy", "std::copy"}, {"memset", "std::fill"}})) {
+Replacements.insert(KeyValue);

Try just initializer list instead of creating vector here.
Just 
for (const auto  : {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.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:98-99
@@ +97,4 @@
+  const auto it = Replacements.find(MatchedName);
+  if (it == Replacements.end())
+return;
+  const auto ReplacedName = it->second;

this should not happen. You can change it to assert


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:139
@@ +138,3 @@
+// Cannot do pointer arithmetic on void type.
+if (getStrippedType(Arg1Type)->isVoidType())
+  return;

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")


Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:37
@@ +36,2 @@
+std::fill(dest, dest + count, ch)
+

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Jonas Devlieghere via cfe-commits]

JDevlieghere retitled this revision from "[clang-tidy] Add check 
'misc-replace-memcpy'" to "[clang-tidy] Add check 'modernize-use-algorithm'".
JDevlieghere updated the summary for this revision.
JDevlieghere set the repository for this revision to rL LLVM.
JDevlieghere updated this revision to Diff 66247.
JDevlieghere added a comment.

- 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


Repository:
  rL LLVM

https://reviews.llvm.org/D22725

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseAlgorithmCheck.cpp
  clang-tidy/modernize/UseAlgorithmCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-algorithm.rst
  test/clang-tidy/modernize-use-algorithm.cpp

Index: test/clang-tidy/modernize-use-algorithm.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-algorithm.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+
+// CHECK-FIXES: #include 
+
+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 
+OutputIt copy(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+OutputIt move(InputIt first, InputIt last, OutputIt d_first);
+
+template 
+void fill(ForwardIt first, ForwardIt last, const T );
+}
+
+void a() {
+  char foo[] = "foo", bar[3];
+  std::memcpy(bar, foo, sizeof bar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use std::copy instead of memcpy [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: Use std::copy instead of memcpy [modernize-use-algorithm]
+
+  std::copy(foo, foo + sizeof bar, bar);
+}
+
+void b() {
+  char foo[] = "foobar";
+  std::memset(foo, 'a', 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use std::fill instead of memset [modernize-use-algorithm]
+  // CHECK-FIXES: std::fill(foo, foo + 3, 'a');
+
+  std::memset(foo, 1, 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use std::fill instead of memset [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: Use std::copy instead of memcpy [modernize-use-algorithm]
+}
Index: docs/clang-tidy/checks/modernize-use-algorithm.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-algorithm.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - modernize-use-algorithm
+
+modernize-use-algorithm
+===
+
+Replaces calls to ``memcpy`` and ``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.
+
+Furthermore, 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)
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -105,6 +105,7 @@
modernize-shrink-to-fit
modernize-use-auto
modernize-use-bool-literals
+   modernize-use-algorithm
modernize-use-default
modernize-use-emplace
modernize-use-nullptr
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -69,6 +69,12 @@
 
   Flags classes where some, but not all, special member functions are user-defined.
 
+- New `modernize-use-algorithm
+  `_ check
+
+  Replaces calls to ``memcpy`` and ``memset`` with their respective algorithm
+  counterparts ``std::copy`` and ``std::fill``.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseAlgorithmCheck.h
===
--- /dev/null
+++