On Thu, May 9, 2013 at 6:24 PM, Richard Smith <[email protected]> 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 <[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
