<div dir="ltr">On Fri, Oct 4, 2013 at 6:05 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Looking pretty good, though Richard should probably give final approval now. Is it worth checking that the string is actually a pointer to some kind of character, though? It's unlikely that you'd add a character to an int*, but the message is a bit off, then.<br>
</blockquote><div><br></div><div>I think the check makes some sense. If I have</div><div><br></div><div>  int *my_lookup_table;</div><div>  int *digit_table = my_lookup_table + '0';</div><div><br></div><div>... then I don't think I want a warning. If the lookup table contains 'char's then this looks a lot more questionable.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, the type in the message should be the type of the character literal (i.e. L'a', u'a', and U'a'), not a hardcoded "char". Unfortunately, in C that type is 'int', so in the C case you should check and see if the value fits in a char, and substitute CharTy in that case.</blockquote>
<div><br></div><div>The idea and the patch both seem good to me, other than the diagnostic wording. Perhaps something like:</div><div><br></div><div>  adding %0 to %1 does not append to the string</div><div><br></div><div>
where %0 and %1 are the types of the LHS and RHS of the +, respectively (with the type tweak Jordan pointed out for C)?</div><div><br></div><div>I wonder whether we want to catch more than just DeclRefExprs on the pointer side. Other interesting cases include CallExprs and MemberExprs. In what cases would we want to suppress the warning based on the syntax of the pointer side?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888"><br>
Jordan<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On Oct 4, 2013, at 5:28 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>> wrote:<br>
<br>
> Hi,<br>
><br>
> A new iteration on the checker. I have moved it to be a warning in semaexpr now.<br>
><br>
> //Anders<br>
><br>
> .......................................................................................................................<br>
> Anders Rönnholm Senior Engineer<br>
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden<br>
><br>
> Mobile:                    <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254">+46 (0)70 912 42 54</a><br>
> E-mail:                    <a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a><br>
><br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br>
><br>
> ________________________________________<br>
> Från: Jordan Rose [<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>]<br>
> Skickat: den 2 oktober 2013 18:12<br>
> Till: Daniel Marjamäki<br>
> Cc: Richard Smith; Anders Rönnholm; cfe commits<br>
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker StringPlusChar<br>
><br>
> On Oct 1, 2013, at 22:16 , Daniel Marjamäki <<a href="mailto:Daniel.Marjamaki@evidente.se">Daniel.Marjamaki@evidente.se</a>> wrote:<br>
><br>
>><br>
>> Hello!<br>
>><br>
>>> This should be a warning, not a static analyser check, shouldn't it?<br>
>><br>
>> Is anybody against this?<br>
>><br>
>> Personally I feel very confident about this check regarding signal/noise. However putting it in the static analyser to start with felt like a safe choice.<br>
><br>
> I'm fine with this. I didn't realize at first it was just for character literals; with that restriction this does seem like false positives would be unlikely. Thanks, Richard. (Sorry to force another iteration, Anders. The existing warning is in SemaExpr.cpp: diagnoseStringPlusInt.)<br>

><br>
> Daniel, delaying += and char plus string seem fine to me, though they might just be handled if you hook into diagnoseStringPlusInt. And thanks for the reminder about things like s8, u8, etc.<br>
><br>
> Jordan<br>
</div></div><br><br>
<br></blockquote></div><br></div></div>