Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer <fweimer 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