[cfe-commits] r148324 - in /cfe/trunk: include/clang/Sema/Sema.h lib/Sema/SemaChecking.cpp lib/Sema/SemaExprObjC.cpp test/SemaObjC/format-strings-objc.m

[Nico Weber]

On Mon, Jan 30, 2012 at 11:26 AM, Jean-Daniel Dupas
<devlists at shadowlab.org> wrote:
>
> Le 30 janv. 2012 à 18:42, Nico Weber a écrit :
>
>> On Mon, Jan 30, 2012 at 9:37 AM, Nico Weber <thakis at chromium.org> wrote:
>>> On Mon, Jan 30, 2012 at 9:32 AM, Jean-Daniel Dupas
>>> <devlists at shadowlab.org> wrote:
>>>>
>>>> Le 30 janv. 2012 à 17:37, Nico Weber a écrit :
>>>>
>>>>> Cool!
>>>>>
>>>>> One thing that this warned on in the chromium source was:
>>>>>
>>>>> ../../webkit/plugins/npapi/plugin_web_event_converter_mac.mm:213:37:
>>>>> error: format specifies type 'wchar_t *' (aka 'wchar_t *') but the
>>>>> argument has type 'const WebUChar *' (aka 'const unsigned short *')
>>>>> [-Werror,-Wformat]
>>>>>       [NSString stringWithFormat:@"%S", key_event.text]);
>>>>>                                    ~^   ~~~~~~~~~~~~~~
>>>>>
>>>>> It looks like the current %S warning checks if the parameter type is
>>>>> wchar_t. http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html#//apple_ref/doc/uid/TP40004265
>>>>> says that %S is always utf16 (or ucs16, it doesn't say). `man 3
>>>>> printf` says %S is for wchar_t, but I guess it refers to the wchar_t
>>>>> type that was effective when libc / AppKit was built. On my OS X 10.6
>>>>> box, it looks like printf expects a four byte wchar_t, while AppKit
>>>>> expects a two byte wchar_t. Since that code snippet above is about
>>>>> AppKit, the code is right and the warning is wrong (chromium isn't
>>>>> compiled with -fshort-wchar, but AppKit was, and the warning really
>>>>> needs to check against the wchar_t type that was used to build the
>>>>> library it's talking to).
>>>>>
>>>>> Since %S can expect utf32 (lib) or utf16 (AppKit), the %S warning is
>>>>> currently fairly useless. %S seems to be an Apple extension, so should
>>>>> we just hardcode %S to look for a 32bit type in c strings and for a
>>>>> 16bit type in @"" strings?
>>>>> Should we disable the warning for %S
>>>>> altogether?
>>>>
>>>>
>>>> I think we should just extends the printf checker to handle %S differently when the format spec is 'NSString' (it already has special handling for %@).
>>>
>>> The current %S warning implementation is wrong for just printf() as
>>> well: It compares the argument to wchar_t, but if you build your
>>> program with -fshort-wchar, clang won't warn (since your program uses
>>> wchar_t) yet printf won't work (because libc was built without
>>> -fshort-wchar). If you store your characters in an uint32* and pass
>>> that to printf() and build your program with -fshort-wchar, clang will
>>> warn (because uint32* doesn't match wchar_t* with -fshort-whar) yet
>>> the program will work correctly.
>>
>> How about: We support it for format spec NSString and expect the type
>> to be some 16bit type, and we always warn on %S else? (It can't be
>> portable elsewhere, and as Hans said, the Ubuntu manpage even says "Do
>> not use.")
>
> Yes, but it says to use %ls instead, which has the same semantic. Moreover, %S is supported on BSD (and so on OS X).
>
> I think we should continue to treat %S and %ls as pointer to wchar_t (as documented) and ignore compatibility issue introduce by -fshort-wchar.
> The -fshort-wchar flag will almost always produce broken code anyway, as it breaks all calls to functions that use a wchar_t arguments, unless the user make sure he uses a lib c compiled with this flag too.
> And when the library and the code both use -fshort-wchar, clang properly handle '%S'.

Ok, I'll prepare a patch for your suggestion then (change %S to expect
16 bit types in nsstring types, don't change the 'normal' current
behavior).

Thanks!