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