[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
<piotr.padlewski at gmail.com> wrote:
>
>
> 2016-08-08 8:33 GMT-07:00 Aaron Ballman <aaron.ballman at gmail.com>:
>>
>> 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 &KeyValue :
>> +       {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
>>
>>
>>
>