<br><br><div class="gmail_quote">Le 29 janvier 2012 00:11, Jean-Daniel Dupas <span dir="ltr"><<a href="mailto:devlists@shadowlab.org">devlists@shadowlab.org</a>></span> a écrit :<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Le 26 janv. 2012 à 18:49, Jean-Daniel Dupas a écrit :<br>
<br>
><br>
> Le 26 janv. 2012 à 18:38, jahanian a écrit :<br>
><br>
>><br>
>> On Jan 26, 2012, at 1:45 AM, Jean-Daniel Dupas wrote:<br>
>><br>
>>><br>
>>> Le 25 janv. 2012 à 22:02, jahanian a écrit :<br>
>>><br>
>>>><br>
>>>> On Jan 25, 2012, at 10:41 AM, Jean-Daniel Dupas wrote:<br>
>>>><br>
>>>>><br>
>>>>> Le 25 janv. 2012 à 18:33, jahanian a écrit :<br>
>>>>><br>
>>>>>><br>
>>>>>> On Jan 24, 2012, at 4:39 PM, Jean-Daniel Dupas wrote:<br>
>>>>>><br>
>>>>>>> Hi,<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> 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.<br>

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

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

>>>>>>> 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: %@").<br>

>>>>>>> Moreover, NSLocalizedString() does not use the "format_arg" attribute.<br>
>>>>>>><br>
>>>>>><br>
>>>>>> 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.<br>
>>>>><br>
>>>>> 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).<br>
>>>>> I attach an updated version of the remaining patch with this issue fixed.<br>
>>>><br>
>>>> This patch is OK for the purpose you mentioned.<br>
>>><br>
>>> Thanks for the reviews.<br>
>>><br>
>>> This patch depends on the first one though, so I will apply it when the first one (attached to this mail) is approved.<br>
>>><br>
>><br>
>> I see. Patch is OK. However, there may be concern about performance of passing a StringRef down. Can you instead pass an<br>
>> enum value of routine names you are interested in?<br>
>><br>
>> - Fariborz<br>
><br>
><br>
> OK. I will have a look at it.<br>
<br>
Here is a new patch that use an enum instead of the StringRef. Does it match what you where thinking about ?<br>
<br>
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 ?<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
-- Jean-Daniel<br>
</font></span></blockquote><div><br>Hi Jean-Daniel,<br><br>First of all, the llvm::StringSwitch is limited in that it can only performs a conversion from a "string" to a value (whatever it is). It cannot perform extraneous actions, such as defining some flags on the way.<br>
<br>When that is said, it really is a matter of readability as far as I am concerned, when you read a "switch" it is immediately clear what is switch on (and thus what affect the choice of branch), and thus the llvm::StringSwitch makes it very clear that it is just a mapping from string to value, at a glance. I do not think (given its implementation) that there is any performance enhancement compared to a basic if/else if/else chain.<br>
<br>-- Matthieu <br></div></div><br>