[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 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?


> ================
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160808/b269703f/attachment.html>