[PATCH] [StaticAnalyzer] New checker StringPlusChar

[Richard Smith]

On Fri, Oct 4, 2013 at 6:05 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> 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.
>

I think the check makes some sense. If I have

  int *my_lookup_table;
  int *digit_table = my_lookup_table + '0';

... then I don't think I want a warning. If the lookup table contains
'char's then this looks a lot more questionable.


> 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.


The idea and the patch both seem good to me, other than the diagnostic
wording. Perhaps something like:

  adding %0 to %1 does not append to the string

where %0 and %1 are the types of the LHS and RHS of the +, respectively
(with the type tweak Jordan pointed out for C)?

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?


> Jordan
>
>
> On Oct 4, 2013, at 5:28 , Anders Rönnholm <Anders.Ronnholm at evidente.se>
> wrote:
>
> > Hi,
> >
> > A new iteration on the checker. I have moved it to be a warning in
> semaexpr now.
> >
> > //Anders
> >
> >
> .......................................................................................................................
> > Anders Rönnholm Senior Engineer
> > Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> >
> > Mobile:                    +46 (0)70 912 42 54
> > E-mail:                    Anders.Ronnholm at evidente.se
> >
> > www.evidente.se
> >
> > ________________________________________
> > Från: Jordan Rose [jordan_rose at apple.com]
> > Skickat: den 2 oktober 2013 18:12
> > Till: Daniel Marjamäki
> > Cc: Richard Smith; Anders Rönnholm; cfe commits
> > Ämne: Re: [PATCH] [StaticAnalyzer] New checker StringPlusChar
> >
> > On Oct 1, 2013, at 22:16 , Daniel Marjamäki <
> Daniel.Marjamaki at evidente.se> wrote:
> >
> >>
> >> Hello!
> >>
> >>> This should be a warning, not a static analyser check, shouldn't it?
> >>
> >> Is anybody against this?
> >>
> >> 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.
> >
> > 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.)
> >
> > 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.
> >
> > Jordan
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131004/89ee7aa1/attachment.html>