[Patch] Better diagnostics for string initialization (and fix for C11)

[Richard Smith]

On Wed, May 15, 2013 at 4:08 AM, Hans Wennborg <hans at chromium.org> wrote:

> On Tue, May 14, 2013 at 9:37 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > @@ -79,7 +117,7 @@ static Expr *IsStringInit(Expr *init, QualType
> declType,
> > ASTContext &Context) {
> >    const ArrayType *arrayType = Context.getAsArrayType(declType);
> >    if (!arrayType) return 0;
> >
> > -  return IsStringInit(init, arrayType, Context);
> > +  return IsStringInit(init, arrayType, Context) == SIF_None ? init : 0;
> >
> > Looks like the one and only caller of this doesn't actually want the
> > expression; maybe just return the SIF value here too?
>
> I've changed it to just return bool since that's really what the
> caller is looking for.


The overload set for IsStringInit is now pretty scary: one overload returns
zero (as an enum) on success and the other returns true, and the only
difference in their signatures is that one takes a 'const ArrayType*' where
the other takes a 'QualType'. Please make them both return the same type.

> @@ -4307,9 +4363,10 @@ InitializationSequence::InitializationSequence(Sema
> > &S,
> >        TryListInitialization(S, Entity, Kind,
> > cast<InitListExpr>(Initializer),
> >                              *this);
> >        AddParenthesizedArrayInitStep(DestType);
> > -    } else if (DestAT->getElementType()->isAnyCharacterType())
> > +    } else if (DestAT->getElementType()->isAnyCharacterType() &&
> > +               !isa<StringLiteral>(Initializer->IgnoreParens())) {
> >        SetFailed(FK_ArrayNeedsInitListOrStringLiteral);
> >
> > I don't think this is right -- the diagnostic should depend on whether
> the
> > element type could be initialized by a string literal, and not on whether
> > the initializer actually was a string literal. That is, in MS C mode,
> >
> >   __wchar_t str[] = xxxx;
> >
> > should not suggest initializing with a string literal, whether xxxx is a
> > string literal or not.
>
> Ah, you're right, it's really the isAnyCharacterType() part that's
> wrong. I've changed this, and now it means we also get better
> diagnostics for code like:
>
>   wchar_t s[] = 1;
>
> in C, where wchar_t is not built-in and thus not part of
> "isAnyCharacterType()".
>

Thanks!


> > Otherwise, looks great, thanks.
>
> Thanks! Committed r181880.
>
>  - Hans
>
>
> > On Tue, May 14, 2013 at 9:48 AM, Hans Wennborg <hans at chromium.org>
> wrote:
> >>
> >> On Tue, May 14, 2013 at 4:12 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >> > On May 13, 2013 9:59 AM, "Hans Wennborg" <hans at chromium.org> wrote:
> >> >>   /tmp/a.c:3:9: error: initializing wide char array with non-wide
> >> >> string
> >> >> literal
> >> >>   wchar_t s[] = "Hi";
> >> >
> >> > Worth it/possible/convenient to go one step further and provide a
> fixit
> >> > to
> >> > insert the 'L' and appropriately recover? I suppose that's probably
> >> > orthogonal to your patch, though.
> >>
> >> I can look into it, but as you say, probably orthogonal to this patch.
> >>
> >> Thanks,
> >> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130515/ab2ebaf4/attachment.html>