On 08/09/2012 06:17 PM, Eric Blake wrote:
> virCasprintf() seems like overkill, for now. Since printing a floating
> point value is the only case where locale matters, we should be able to
> provide a single helper function that guarantees a formatted float,
> rather than trying to provide a generic virCasprintf that covers all
> possible format strings. I don't even think we need to worry about
> things like 0 padding, justification, or precision - either we are
> outputting things to the user (where alignment and precision make things
> look nice, but then we WANT to obey locale), or we are converting to an
> internal string (where we want the string to be reproducible, and
> worrying about formatting is unneeded bulk). That is, I think we are
> better off with this signature in util.c:
>
> /* Return a malloc'd string representing D in the C locale, or NULL on
> OOM. */
> char *virDoubleToStr(double d);
>
> (and possibly virFloatToStr and virLongDoubleToStr if we ever have
> reason for them later on; or even add an option argument to let the user
> choose between a, e, f, or g format). At any rate, the fallback code
> for replacing the decimal character should live in util.c, not in the
> clients.
>
I see I should've stuck with one of my versions and don't listen to guys
around here ;-) I'm with you on this one.
>> + if (ret == -1) {
>> return NULL;
>> + } else if (ret == -2) {
>> + char *radix, *tmp;
>> + struct lconv *lc;
>
> I'm still worried about whether 'struct lconv' will compile on mingw.
> Then again, any system that lacks localeconf() probably also lacks any
> locale that would use ',' for the decimal separator, so maybe
> appropriate ifdef protection is all we need.
>
As we based it on glib code, I'd say: "they have it like this" :)
>> @@ -2003,6 +2004,47 @@ virAsprintf(char **strp, const char *fmt, ...)
>> }
>>
>> /**
>> + * virCasprintf
>> + *
>> + * the same as virAsprintf, but with C locale (thread-safe)
>> + *
>> + * If thread-safe locales aren't supported, then the return value is -2,
>> + * no memory (or other vasnprintf errors) results in -1.
>> + * When successful, the value returned by virAsprintf is passed.
>> + */
>> +#if HAVE_XLOCALE_H
>
> And where does HAVE_XLOCALE_H get defined? Autoconf conventions say it
> would map to <xlocale.h> existing, but that is a non-standard header
> name. Not to mention that we really care about whether newlocale and
> setlocale exist.
>
I tried to create a have_xlocale with AC_COMPILE_IFELSE but since
uselocale, newlocale and freelocale are part of libc, I haven't managed
to make it fail. Even after removing all the header files the only
problem was with NULL, but no problem with these function. I'll recreate
what I threw away and I'll send it as a new version and in the meantime
I'll try to find a machine/configuration the test will fail on.
>> +int
>> +virCasprintf(char **strp, const char *fmt, ...)
>> +{
>> + va_list ap;
>> + int ret = -1;
>> + locale_t c_loc, old_loc;
>> + c_loc = newlocale(LC_ALL, "C", NULL);
>
> This is expensive to recreate on the fly for every caller; I think it
> should be a static variable and created only once (the virInitOnce stuff
> makes the most sense here).
>
OK, will do. Thanks for the review and ideas, I'll jump right on that ;)
Martin
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list