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

[Hans Wennborg]

On Thu, May 9, 2013 at 6:24 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> > 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"
>
>
> 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).

I agree. I have started looking at this in a separate patch.

>> 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.
>
>
> CGRTTI.cpp: This should be WCharTy (note that unsigned int and unsigned char
> are also in the list).
> ASTReader: Correct, use WCharTy here (see ASTCommon.cpp for the
> corresponding serialization code).
> SemaOverload.cpp: This should be WCharTy (note that unsigned int and
> unsigned char are also in the list).

I've updated these, and unless there are any other issues I will
commit tomorrow morning when I have time to watch the buildbots.

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