[Patch] Add the __wchar_t type in MS-compatibility mode (PR15815)

[Hans Wennborg]

Thanks for the review!

On Wed, May 8, 2013 at 9:55 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Is it essential to define __wchar_t as a separate keyword and TST rather
> than just reusing wchar_t? More generally, I think I'd prefer to see
> ASTContext have a 'WCharType', which is always the same as __wchar_t, and is
> also the same as wchar_t when it's a keyword, and also a 'WideCharType',
> which is the type of wide characters and wide strings.

You're right! The key thing is that we need to have two different
types in ASTContext. The names you suggest sound good to me. We can
get rid off the separate keyword and TST and the patch becomes less
intrusive.

> What's the change in SemaInit.cpp for? It looks like it's a diagnostic
> improvement for the case where you try to initialize an array of character
> type from a string literal of an incompatible type. If this is desirable,
> could it be committed separately? Also, should it be using IsStringInit
> rather than isa<StringLiteral>? Right now, it seems to be missing a check
> for ObjCEncodeExpr.

It was for this case in C:

  __wchar_t s[] = L"foo";

Where Clang would say: "array initializer must be an initializer list
or string literal" which was weird when the right hand side is already
a string literal. I think we can work on this diagnostic in a separate
patch; for example, we get the same diagnostic for this: char s[] =
L"foo"


Attaching a new patch. I've changed most uses of WCharTy to
WideCharTy, but I have marked some cases with XXX where I wasn't
completely sure. It would be great if you could double check those.

Thanks,
Hans


> On Wed, May 8, 2013 at 9:20 AM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> Ping?
>>
>> On Mon, May 6, 2013 at 4:02 PM, Hans Wennborg <hans at chromium.org> wrote:
>> > Hello again,
>> >
>> > As it turned out, that commit broke test/Sema/wchar.c on Windows, and
>> > was reverted.
>> >
>> > The line that failed was this:
>> >
>> >   unsigned short s[] = L"something";
>> >
>> > The problem is that my patch was causing the type of the string
>> > literal to change to __wchar_t on Windows, and since that is a
>> > different type than unsigned short, the initialization doesn't work.
>> >
>> > I experimented some more with MSVC and learned that the code above
>> > does compile in C mode, but not in C++ mode. The following does not
>> > compile in C mode:
>> >
>> >   __wchar_t s[] = L"something";
>> >
>> > Which I found surprising.
>> >
>> > I think the semantics are like this:
>> >
>> > In C++, we have the wchar_t built-in type, and that's the type used
>> > for wide string literals. __wchar_t is the same as wchar_t.
>> >
>> > In C, __wchar_t is the same as the built-in wchar_t would have been if
>> > it were available. The type of wide string literals is array of
>> > unsigned short.
>> >
>> > I'm attaching a new patch that implements this behavior. Please take a
>> > look, and sorry again for the breakage.
>> >
>> > Thanks,
>> > Hans
>> >
>> > On Fri, May 3, 2013 at 10:16 AM, Hans Wennborg <hans at chromium.org>
>> > wrote:
>> >> Thanks! Committed r181004.
>> >>
>> >> On Thu, May 2, 2013 at 6:55 PM, Aaron Ballman <aaron at aaronballman.com>
>> >> wrote:
>> >>> Patch LGTM!
>> >>>
>> >>> ~Aaron
>> >>>
>> >>> On Thu, May 2, 2013 at 1:38 PM, Hans Wennborg <hans at chromium.org>
>> >>> wrote:
>> >>>> On Tue, Apr 23, 2013 at 4:46 PM, Hans Wennborg <hans at chromium.org>
>> >>>> wrote:
>> >>>>> On Tue, Apr 23, 2013 at 2:48 PM, Richard Smith
>> >>>>> <richard at metafoo.co.uk> wrote:
>> >>>>>> Does the setup code in ASTContext::InitBuiltinTypes do the right
>> >>>>>> thing here?
>> >>>>>
>> >>>>> Hmm, turns out it didn't.
>> >>>>>
>> >>>>> I guess it's not obvious what the right thing is here. From
>> >>>>> experimenting a bit, it seems that __wchar_t is always available,
>> >>>>> and
>> >>>>> is always a distinct builtin type in visual studio, even in C.
>> >>>>>
>> >>>>> New patch attached.
>> >>>>
>> >>>> Richard pointed out on IRC that we shouldn't change semantics in
>> >>>> -fms-extensions.
>> >>>>
>> >>>> I'm attaching a new patch. In -fms-extensions, __wchar_t is the same
>> >>>> as built-in wchar_t if available, otherwise it is the same as the
>> >>>> appropriate integer type.
>> >>>>
>> >>>> In -fms-compatibility we try to mimic MSVC exactly: there is always a
>> >>>> __wchar_t type, and it is always separate from the regular integer
>> >>>> types.
>> >>>>
>> >>>> There are a number of parameters here: C vs. C++, -fms-extensions vs.
>> >>>> -fms-compatibility, and -fno-wchar. The patch covers all of them and
>> >>>> I
>> >>>> think the tests make it reasonably clear. If we think this is too
>> >>>> complicated, we could just use only the -fms-extensions part of the
>> >>>> patch.
>> >>>>
>> >>>> New patch attached, please take a look.
>> >>>>
>> >>>> Thanks,
>> >>>> Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wchar_t5.patch
Type: application/octet-stream
Size: 15701 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130509/32895782/attachment.obj>