<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Thanks for taking a look!</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Sat, Dec 21, 2013 at 2:26 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On 21/12/2013 07:31, Nico Weber wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi,<br>
<br>
the attached patch adds a new warning that makes memcmp & co warn on misplaced parentheses such as<br>
<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
<br>
like so:<br>
<br>
test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison [-Wmemsize-comparison]<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
~~~~~~~~~~^~~~<br>
</blockquote>
<br>
<br></div>
Hi Nico,<br>
<br>
Agree we'd do well to diagnose here, but it seems can get a lot more mileage by simplifying and generalizing the check.<br>
<br>
This doesn't appear to be a memsize-specific problem, nor is it necessarily limited to binop argument expressions.<br>
<br>
Surely it's rather the case that any implicit bool-to-size_t call argument conversion is at best dubious, and most likely a bug?</blockquote><div><br></div><div>I think it's not obvious which direction the generalization should go: You suggest implicit bool-to-size_t – there's a much higher chance for false positives there. If experiments show that this is in fact viable, should this be done for bool-to-unsigned? What about bool-to-int – we already know that this has a noise level that's too high to be bearable. Would you do it everywhere, or only in calls?</div>
<div><br></div><div>I think a more promising generalization is to do this for the last parameter of calls whose last parameter type is unsigned.</div><div><br></div><div>But since it's not clear, I figured this is a generally agreeable and useful subset, so I wanted to get this in first; this is what I meant with "this can possibly be extended later on<span style="font-family:arial,sans-serif;font-size:13px">, but I feel this is a good start".</span></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im"><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
test4.cc:5:7: note: did you mean to compare the result of 'memcmp' instead?<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
^ ~<br>
)<br>
test4.cc:5:20: note: explicitly cast the argument to size_t to silence this warning<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
^<br>
(size_t)( )<br>
</blockquote>
<br>
<br></div>
A cast is the wrong FixIt to suggest. "sizeof(a) ? 1 : 0" is closer to the natural way to write this construct if it was indeed their true intention.<br></blockquote><div><br></div><div>This is mostly a style thing, no? Is there any technical reason to prefer your suggestion? I went with a cast since several other warnings have a "explicitly cast to suppress" note, and I liked the consistency in diagnostics. (?1:0 will suppress the warning too, so people can use that if they prefer this.)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">However it's best not to generate a FixIt in this context given that we have a high degree of confidence that the user's code is bogus, especially since the FixIt hides away the bug into a form that's harder to detect.<br>
</blockquote><div><br></div><div>Note that it's a fixit on a note, not on the diagnostic itself, so it's never applied automatically. We have several diagnostics with fixits on the "to suppress" note. (See also the bit on fixits on <a href="http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints">http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints</a>).</div>
<div><br></div><div>Nico</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Alp.<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
1 warning generated.<br>
<br>
<br>
This would have found one bug in NSS that we recently fixed [1] and found one more bug in chromium we didn't know about before [2]. It had 0 false positives on all of chromium.<br>
<br>
The idea of triggering this warning on a binop in the size argument is due to rnk.<br>
<br>
This warning can possibly be extended later on, but I feel this is a good start.<br>
<br>
Thoughts?<br>
<br>
Nico<br>
<br>
[1]: <a href="https://codereview.chromium.org/99423002/diff/1/net/third_party/nss/ssl/ssl3con.c" target="_blank">https://codereview.chromium.<u></u>org/99423002/diff/1/net/third_<u></u>party/nss/ssl/ssl3con.c</a><br>
[2]: <a href="https://codereview.chromium.org/8431007/#msg12" target="_blank">https://codereview.chromium.<u></u>org/8431007/#msg12</a><br>
<br>
<br></div>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><span class=""><font color="#888888"><br>
</font></span></blockquote><span class=""><font color="#888888">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</font></span></blockquote></div><br></div></div>