This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]
- From: Florian Weimer <fweimer at redhat dot com>
- To: Martin Sebor <msebor at redhat dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Mon, 7 Mar 2016 14:22:55 +0100
- Subject: Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]
- Authentication-results: sourceware.org; auth=none
- References: <56CF62D0 dot 8060803 at redhat dot com> <56CF7D30 dot 2090000 at redhat dot com>
On 02/25/2016 11:16 PM, Martin Sebor wrote:
> On 02/25/2016 01:23 PM, Florian Weimer wrote:
>> strfmon_l incorrectly applied global locale settings to number
>> formatting because printf_fp could only look at the global locale.
>>
>> The new test tries to exercise the (in)dependence of a few LC_MONETARY
>> properties. Some of the locales I used for testing might be buggy.
>>
>> I introduced new helper macros to index the locale data in a locale_t
>> object. A future cleanup opportunity is the _NL_CURRENT redefinition in
>> strfmon_l itself. __guess_grouping should be moved out of printf_fp.c,
>> with a proper prototype in a header file.
>>
>> I saw some ldbl_hidden_def thing. I fear it is used by ld-as-a-functor
>> magic to work around the lack of templates in C. What I did should
>> hopefully be safe.
>>
>> Martin, does this patch match what you had in mind?
>
> It's very close to what I started with. I had made some slightly
> different choices in my initial approach, some of which would have
> probably turned out to be incorrect in the end. I like that you
> added inline functions rather than macros to access the locale
> data. That's a lot cleaner.
Thanks for your comments.
Yes, it makes the types nicely explicit.
> The one thing that I'm curious about is the two sets of
> _NL_CURRENT_XXX() macros defined in localeinfo.h and controlled
> by NL_CURRENT_INDIRECT. If I'm reading your patch right it
> assumes NL_CURRENT_INDIRECT is undefined. Does it still work
> as expected when NL_CURRENT_INDIRECT is defined? (I never got
> far enough along to test this with my patch.)
I don't think it does. The representation of locale_t does not really
change due to static linking.
I have added a static test, like this:
/* Test locale dependence of strfmon_l, static variant.
Copyright (C) 2016 Free Software Foundation, Inc.
This file is part of the GNU C Library.
â
You should have received a copy of the GNU Lesser General Public
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
#include "tst-strfmon_l.c"
stdlib/tst-strfmon_l-static: ELF 64-bit LSB executable, x86-64, version
1 (GNU/Linux), statically linked, for GNU/Linux 2.6.32,
BuildID[sha1]=a78456f193cfcd0b04912f55654c7cd342edca24, not stripped
And it still passes. I can include it in the change if you think it's
necessary.
> Other than that, I have one suggestion for the test. I don't
> know if it would be in line with the glibc approach to testing
> (and it's also a matter of personal preference), but for what
> it's worth, I find data/table driven tests easier to read, work
> with, and enhance. I.e., rather than calling
>
> check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
> "%i", "USD 1,234,567.89");
>
> a bunch of times with varying formats and data, I find that
> defining an array of structs containing the data and then
> iterating over the data makes it easier to see the important
> bits under test and keep the unimportant parts out of sight:
I did not do this to avoid a warning when building with -Wformat-nonliteral.
Florian