[cfe-commits] [PATCH] format attribute improvements.

[Jean-Daniel Dupas]

Le 26 janv. 2012 à 18:49, Jean-Daniel Dupas a écrit :

> 
> Le 26 janv. 2012 à 18:38, jahanian a écrit :
> 
>> 
>> On Jan 26, 2012, at 1:45 AM, Jean-Daniel Dupas wrote:
>> 
>>> 
>>> Le 25 janv. 2012 à 22:02, jahanian a écrit :
>>> 
>>>> 
>>>> On Jan 25, 2012, at 10:41 AM, Jean-Daniel Dupas wrote:
>>>> 
>>>>> 
>>>>> Le 25 janv. 2012 à 18:33, jahanian a écrit :
>>>>> 
>>>>>> 
>>>>>> On Jan 24, 2012, at 4:39 PM, Jean-Daniel Dupas wrote:
>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> 
>>>>>>> 3-macros: In Obj-C and CoreFoundation, the recommended way to localize string are respectively to use NSLocalizedString(key, comment) and CFCopyLocalizedString(key, comment) macros family.
>>>>>>> It is common to use these macros as format string, but as they expand to method/function call, clang will warn about "non literal string" used as format string.
>>>>>>> So, this patch is a tentative to prevent diagnostic for this common usage. It inhibits the "non literal string format" diagnostic when the format type if NS/CFstring and the format argument is a macro expansion.
>>>>>>> Note that while the CFCopyLocalizedString() expands to a function properly tagged with the "format_arg" attribute, we can't rely on it, because interpreting the 'key' parameter as a format string is incorrect IMHO.
>>>>>>> It is a common practice to use some kind of descriptive name for the key (i.e. "UNEXPECTED_ERROR_TITLE") instead of the string value ("An unexpected error occurred: %@").
>>>>>>> Moreover, NSLocalizedString() does not use the "format_arg" attribute.
>>>>>>> 
>>>>>> 
>>>>>> I don't seem to be getting the warning on the test case, and I don't think you have yet checked in the patch.
>>>>> 
>>>>> You're right. The tested warning is not enabled by default, and I forgot to add it to the test command line (or to enable it using pragma diagnostic).
>>>>> I attach an updated version of the remaining patch with this issue fixed.
>>>> 
>>>> This patch is OK for the purpose you mentioned.
>>> 
>>> Thanks for the reviews.
>>> 
>>> This patch depends on the first one though, so I will apply it when the first one (attached to this mail) is approved.
>>> 
>> 
>> I see. Patch is OK. However, there may be concern about performance of passing a StringRef down. Can you instead pass an
>> enum value of routine names you are interested in?
>> 
>> - Fariborz
> 
> 
> OK. I will have a look at it.

Here is a new patch that use an enum instead of the StringRef. Does it match what you where thinking about ?

I have a question though. What are the pro and cons of the llvm::StringSwitch ? When is it better to use it, and when is it better to keep using if/elseif ?


-- Jean-Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1-type.patch
Type: application/octet-stream
Size: 14154 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120129/6803ea08/attachment.obj>