On Thu, May 9, 2013 at 6:29 AM, Hans Wennborg <[email protected]> wrote:
> Thanks for the review! > > On Wed, May 8, 2013 at 9:55 PM, Richard Smith <[email protected]> > 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 <[email protected]> wrote: > >> > >> Ping? > >> > >> On Mon, May 6, 2013 at 4:02 PM, Hans Wennborg <[email protected]> > 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 <[email protected]> > >> > wrote: > >> >> Thanks! Committed r181004. > >> >> > >> >> On Thu, May 2, 2013 at 6:55 PM, Aaron Ballman < > [email protected]> > >> >> wrote: > >> >>> Patch LGTM! > >> >>> > >> >>> ~Aaron > >> >>> > >> >>> On Thu, May 2, 2013 at 1:38 PM, Hans Wennborg <[email protected]> > >> >>> wrote: > >> >>>> On Tue, Apr 23, 2013 at 4:46 PM, Hans Wennborg <[email protected]> > >> >>>> wrote: > >> >>>>> On Tue, Apr 23, 2013 at 2:48 PM, Richard Smith > >> >>>>> <[email protected]> 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
