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: "Carlos O'Donell" <codonell at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>, Martin Sebor <msebor at redhat dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Mon, 7 Mar 2016 10:28:28 -0500
- 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> <56DD80AF dot 3000408 at redhat dot com>
- Reply-to: carlos at redhat dot com
On 03/07/2016 08:22 AM, Florian Weimer wrote:
> 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.
I tend to agree with Martin here, having that separation between
test and data makes it easier to read and change the test or add
more tests.
If you're getting a warning from the compiler you expect but don't
care about then you can just silence the warning with the appropriate
attribute?
--
Cheers,
Carlos.