On Wed, May 15, 2013 at 4:08 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Tue, May 14, 2013 at 9:37 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> @@ -79,7 +117,7 @@ static Expr *IsStringInit(Expr *init, QualType declType,<br>
> ASTContext &Context) {<br>
>    const ArrayType *arrayType = Context.getAsArrayType(declType);<br>
>    if (!arrayType) return 0;<br>
><br>
> -  return IsStringInit(init, arrayType, Context);<br>
> +  return IsStringInit(init, arrayType, Context) == SIF_None ? init : 0;<br>
><br>
> Looks like the one and only caller of this doesn't actually want the<br>
> expression; maybe just return the SIF value here too?<br>
<br>
</div>I've changed it to just return bool since that's really what the<br>
caller is looking for.</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> @@ -4307,9 +4363,10 @@ InitializationSequence::InitializationSequence(Sema<br>
> &S,<br>
>        TryListInitialization(S, Entity, Kind,<br>
> cast<InitListExpr>(Initializer),<br>
>                              *this);<br>
>        AddParenthesizedArrayInitStep(DestType);<br>
> -    } else if (DestAT->getElementType()->isAnyCharacterType())<br>
> +    } else if (DestAT->getElementType()->isAnyCharacterType() &&<br>
> +               !isa<StringLiteral>(Initializer->IgnoreParens())) {<br>
>        SetFailed(FK_ArrayNeedsInitListOrStringLiteral);<br>
><br>
> I don't think this is right -- the diagnostic should depend on whether the<br>
> element type could be initialized by a string literal, and not on whether<br>
> the initializer actually was a string literal. That is, in MS C mode,<br>
><br>
>   __wchar_t str[] = xxxx;<br>
><br>
> should not suggest initializing with a string literal, whether xxxx is a<br>
> string literal or not.<br>
<br>
</div>Ah, you're right, it's really the isAnyCharacterType() part that's<br>
wrong. I've changed this, and now it means we also get better<br>
diagnostics for code like:<br>
<br>
  wchar_t s[] = 1;<br>
<br>
in C, where wchar_t is not built-in and thus not part of "isAnyCharacterType()".<br></blockquote><div><br></div><div>Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

> Otherwise, looks great, thanks.<br>
<br>
Thanks! Committed r181880.<br>
<span class="HOEnZb"><font color="#888888"><br>
 - Hans<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
> On Tue, May 14, 2013 at 9:48 AM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>><br>
>> On Tue, May 14, 2013 at 4:12 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> > On May 13, 2013 9:59 AM, "Hans Wennborg" <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>> >>   /tmp/a.c:3:9: error: initializing wide char array with non-wide<br>
>> >> string<br>
>> >> literal<br>
>> >>   wchar_t s[] = "Hi";<br>
>> ><br>
>> > Worth it/possible/convenient to go one step further and provide a fixit<br>
>> > to<br>
>> > insert the 'L' and appropriately recover? I suppose that's probably<br>
>> > orthogonal to your patch, though.<br>
>><br>
>> I can look into it, but as you say, probably orthogonal to this patch.<br>
>><br>
>> Thanks,<br>
>> Hans<br>
</div></div></blockquote></div><br>