Hi Nico,<div><br><div class="gmail_quote">On Tue, Feb 21, 2012 at 7:17 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
the attached patch adds -Wstring-plus-int (I'm open to more creative<br>
names :-) ), which warns on code like |"error code: " + err|. The<br>
warning found 4 bugs in 2 files in chromium. The bugs it found look<br>
like this:<br>
<br>
File 1:<br>
if (!read_node.InitByIdLookup(it->id)) {<br>
error_handler()->OnUnrecoverableError(FROM_HERE, "Failed to look up "<br>
" data for received change with id " + it->id);<br>
return;<br>
}<br>
<br>
File 2:<br>
ResetStream(stream_id, spdy::INVALID_ASSOCIATED_STREAM,<br>
"Received OnSyn with inactive associated stream " +<br>
associated_stream_id);<br>
(Fix: <a href="http://codereview.chromium.org/9372076/diff/1/net/spdy/spdy_session.cc" target="_blank">http://codereview.chromium.org/9372076/diff/1/net/spdy/spdy_session.cc</a> )<br>
<br>
(A coworker found the bug in the one file, which prompted me writing<br>
the warning. The warning then found the bug in the other file.)<br>
<br>
<br>
When building all of chromium and its many dependencies, the warning<br>
did also find 3 false positives, but they all look alike: Ffmepg<br>
contains three different functions that all look like this:<br></blockquote><div><br></div><div>4 bugs and 3 false positives doesn't sound great to me. Have you considered relaxing the warning in cases where the integral summand is a constant expression and is in-bounds? Would this pattern have matched any of your real bugs?</div>
<div><br></div><div>Onto the patch...</div><div><br></div><div><div>Index: include/clang/Basic/DiagnosticGroups.td</div><div>===================================================================</div><div>--- include/clang/Basic/DiagnosticGroups.td<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 150418)</div>
<div>+++ include/clang/Basic/DiagnosticGroups.td<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -320,6 +321,7 @@</div><div> ReturnType,</div><div> SelfAssignment,</div><div>
SizeofArrayArgument,</div><div>+ StringPlusInt,</div><div> Trigraphs,</div><div> Uninitialized,</div><div> UnknownPragmas,</div></div><div><br></div><div>Given the current level of false positives, I'm not completely convinced this should go into -Wmost. I imagine we'll know more once we've run this on more code.</div>
<div><br></div><div><div>Index: include/clang/Basic/DiagnosticSemaKinds.td</div><div>===================================================================</div><div>--- include/clang/Basic/DiagnosticSemaKinds.td<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 150418)</div>
<div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -3414,6 +3414,12 @@</div><div> "explicitly assigning a variable of type %0 to itself">,</div>
<div> InGroup<SelfAssignment>, DefaultIgnore;</div><div> </div><div>+def warn_string_plus_int : Warning<</div><div>+ "Adding an integral to a string does not append to the string">,</div><div><br>
</div><div>Diagnostics outside the static analyzer should not be capitalized.</div><div><br></div><div>+ InGroup<StringPlusInt>;</div><div>+def note_string_plus_int_silence : Note<</div><div>+ "use &(str[index]) if pointer arithmetic is intended">;</div>
<div><br></div><div>The parentheses here are redundant. I think I'd prefer for the suggestion to be provided via fixits (and to reword this as something like "use array indexing to silence this warning").</div>
<div><br></div></div><div><div>Index: lib/Sema/SemaExpr.cpp</div><div>===================================================================</div><div>--- lib/Sema/SemaExpr.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 150418)</div>
<div>+++ lib/Sema/SemaExpr.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -7798,6 +7798,20 @@</div><div> ParensRange);</div><div> }</div><div> </div><div>+/// DiagnoseStringPlusInt - Emit a warning when adding an integer to a string</div>
<div>+/// literal.</div><div>+static void DiagnoseStringPlusInt(Sema &Self, SourceLocation OpLoc,</div><div>+ Expr *LHSExpr, Expr *RHSExpr) {</div><div>+ bool IsStringPlusInt = dyn_cast<StringLiteral>(LHSExpr) &&</div>
<div>+ RHSExpr->getType()->isIntegralType(Self.getASTContext());</div><div><br></div><div>We should probably also check for (unscoped) enumeration types, and for class types with implicit conversion to int. Doing so will require moving this check to checkArithmeticOpPointerOperand (after we've checked for an overloaded operator+).</div>
<div><br></div><div>+ if (!IsStringPlusInt)</div><div>+ return;</div><div>+</div><div>+ SourceRange DiagRange(LHSExpr->getLocStart(), RHSExpr->getLocEnd());</div><div>+ Self.Diag(OpLoc, diag::warn_string_plus_int) << DiagRange;</div>
<div>+ Self.Diag(OpLoc, diag::note_string_plus_int_silence);</div><div>+}</div></div><div><br></div><div>We should also catch the case where the integer is on the LHS (and skip the fixit in that case).</div><div><br></div>
<div>Richard</div></div></div>