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

[Matthieu Monrocq]

Le 29 janvier 2012 00:11, Jean-Daniel Dupas <devlists at shadowlab.org> a
écrit :

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

Hi Jean-Daniel,

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.

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.

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120129/82c67849/attachment.html>