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

[Richard Smith]

On Thu, May 9, 2013 at 6:29 AM, Hans Wennborg <hans at chromium.org> wrote:

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

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


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

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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130509/476f404d/attachment.html>