On Thu, May 9, 2013 at 6:29 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">
Thanks for the review!<br>
<div class="im"><br>
On Wed, May 8, 2013 at 9:55 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> Is it essential to define __wchar_t as a separate keyword and TST rather<br>
> than just reusing wchar_t? More generally, I think I'd prefer to see<br>
> ASTContext have a 'WCharType', which is always the same as __wchar_t, and is<br>
> also the same as wchar_t when it's a keyword, and also a 'WideCharType',<br>
> which is the type of wide characters and wide strings.<br>
<br>
</div>You're right! The key thing is that we need to have two different<br>
types in ASTContext. The names you suggest sound good to me. We can<br>
get rid off the separate keyword and TST and the patch becomes less<br>
intrusive.<br>
<div class="im"><br>
> What's the change in SemaInit.cpp for? It looks like it's a diagnostic<br>
> improvement for the case where you try to initialize an array of character<br>
> type from a string literal of an incompatible type. If this is desirable,<br>
> could it be committed separately? Also, should it be using IsStringInit<br>
> rather than isa<StringLiteral>? Right now, it seems to be missing a check<br>
> for ObjCEncodeExpr.<br>
<br>
</div>It was for this case in C:<br>
<br>
__wchar_t s[] = L"foo";<br>
<br>
Where Clang would say: "array initializer must be an initializer list<br>
or string literal" which was weird when the right hand side is already<br>
a string literal. I think we can work on this diagnostic in a separate<br>
patch; for example, we get the same diagnostic for this: char s[] =<br>
L"foo"<br></blockquote><div><br></div><div>I think there are two separate issues here. One is that our diagnostic for initializing a character array from a string literal of the wrong type is poor. The other is that, in C, we should not treat __wchar_t as a character type for the purpose of this check (because it can never be initialized by a string literal).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Attaching a new patch. I've changed most uses of WCharTy to<br>
WideCharTy, but I have marked some cases with XXX where I wasn't<br>
completely sure. It would be great if you could double check those.<br></blockquote><div><br></div><div>CGRTTI.cpp: This should be WCharTy (note that unsigned int and unsigned char are also in the list).</div><div>ASTReader: Correct, use WCharTy here (see ASTCommon.cpp for the corresponding serialization code).</div>
<div>SemaOverload.cpp: This should be WCharTy (note that unsigned int and unsigned char are also in the list).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks,<br>
Hans<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
> On Wed, May 8, 2013 at 9:20 AM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>><br>
>> Ping?<br>
>><br>
>> On Mon, May 6, 2013 at 4:02 PM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>> > Hello again,<br>
>> ><br>
>> > As it turned out, that commit broke test/Sema/wchar.c on Windows, and<br>
>> > was reverted.<br>
>> ><br>
>> > The line that failed was this:<br>
>> ><br>
>> > unsigned short s[] = L"something";<br>
>> ><br>
>> > The problem is that my patch was causing the type of the string<br>
>> > literal to change to __wchar_t on Windows, and since that is a<br>
>> > different type than unsigned short, the initialization doesn't work.<br>
>> ><br>
>> > I experimented some more with MSVC and learned that the code above<br>
>> > does compile in C mode, but not in C++ mode. The following does not<br>
>> > compile in C mode:<br>
>> ><br>
>> > __wchar_t s[] = L"something";<br>
>> ><br>
>> > Which I found surprising.<br>
>> ><br>
>> > I think the semantics are like this:<br>
>> ><br>
>> > In C++, we have the wchar_t built-in type, and that's the type used<br>
>> > for wide string literals. __wchar_t is the same as wchar_t.<br>
>> ><br>
>> > In C, __wchar_t is the same as the built-in wchar_t would have been if<br>
>> > it were available. The type of wide string literals is array of<br>
>> > unsigned short.<br>
>> ><br>
>> > I'm attaching a new patch that implements this behavior. Please take a<br>
>> > look, and sorry again for the breakage.<br>
>> ><br>
>> > Thanks,<br>
>> > Hans<br>
>> ><br>
>> > On Fri, May 3, 2013 at 10:16 AM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>><br>
>> > wrote:<br>
>> >> Thanks! Committed r181004.<br>
>> >><br>
>> >> On Thu, May 2, 2013 at 6:55 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> >> wrote:<br>
>> >>> Patch LGTM!<br>
>> >>><br>
>> >>> ~Aaron<br>
>> >>><br>
>> >>> On Thu, May 2, 2013 at 1:38 PM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>><br>
>> >>> wrote:<br>
>> >>>> On Tue, Apr 23, 2013 at 4:46 PM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>><br>
>> >>>> wrote:<br>
>> >>>>> On Tue, Apr 23, 2013 at 2:48 PM, Richard Smith<br>
>> >>>>> <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>> >>>>>> Does the setup code in ASTContext::InitBuiltinTypes do the right<br>
>> >>>>>> thing here?<br>
>> >>>>><br>
>> >>>>> Hmm, turns out it didn't.<br>
>> >>>>><br>
>> >>>>> I guess it's not obvious what the right thing is here. From<br>
>> >>>>> experimenting a bit, it seems that __wchar_t is always available,<br>
>> >>>>> and<br>
>> >>>>> is always a distinct builtin type in visual studio, even in C.<br>
>> >>>>><br>
>> >>>>> New patch attached.<br>
>> >>>><br>
>> >>>> Richard pointed out on IRC that we shouldn't change semantics in<br>
>> >>>> -fms-extensions.<br>
>> >>>><br>
>> >>>> I'm attaching a new patch. In -fms-extensions, __wchar_t is the same<br>
>> >>>> as built-in wchar_t if available, otherwise it is the same as the<br>
>> >>>> appropriate integer type.<br>
>> >>>><br>
>> >>>> In -fms-compatibility we try to mimic MSVC exactly: there is always a<br>
>> >>>> __wchar_t type, and it is always separate from the regular integer<br>
>> >>>> types.<br>
>> >>>><br>
>> >>>> There are a number of parameters here: C vs. C++, -fms-extensions vs.<br>
>> >>>> -fms-compatibility, and -fno-wchar. The patch covers all of them and<br>
>> >>>> I<br>
>> >>>> think the tests make it reasonably clear. If we think this is too<br>
>> >>>> complicated, we could just use only the -fms-extensions part of the<br>
>> >>>> patch.<br>
>> >>>><br>
>> >>>> New patch attached, please take a look.<br>
>> >>>><br>
>> >>>> Thanks,<br>
>> >>>> Hans<br>
</div></div></blockquote></div><br>