<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2016-08-08 8:33 GMT-07:00 Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aaron.ballman added inline comments.<br>
<span class=""><br>
================<br>
Comment at: clang-tidy/modernize/<wbr>UseAlgorithmCheck.cpp:59-61<br>
@@ +58,5 @@<br>
+ IncludeStyle(utils::<wbr>IncludeSorter::<wbr>parseIncludeStyle(<br>
+ Options.get("IncludeStyle", "llvm"))) {<br>
+<br>
+ for (const auto &KeyValue :<br>
+ {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}) {<br>
----------------<br>
</span><span class="">alexfh wrote:<br>
> I'm not sure this works on MSVC2013. Could someone try this before submitting?<br>
</span>It does not work in MSVC 2013.<br>
<span class=""><br></span></blockquote><div>What is the best way to fix it and still have a nice code?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
================<br>
Comment at: clang-tidy/modernize/<wbr>UseAlgorithmCheck.cpp:102<br>
@@ +101,3 @@<br>
+ assert(it != Replacements.end() &&<br>
+ "Replacements list does not match list of registered matcher names");<br>
+ const std::string ReplacedName = it->second;<br>
----------------<br>
</span><span class="">alexfh wrote:<br>
> Notes are treated completely differently - each note has to be attached to a warning.<br>
><br>
> 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.<br>
</span>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.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div>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</div><div>C++ programmers who see any of it first time to get it.</div><div>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. </div><div><br></div><div>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.</div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Piotr</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
Repository:<br>
rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D22725" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D22725</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div></div>