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

[Martin Sebor <msebor at redhat dot com>]

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.

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.)

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:

  struct {
      const char *format;
      double value;
      const char *expect;
  } testdata[] = {
      "%i", 1234567.89, "USD 1,234,567.89",
      ...
  };

  for (i = 0; i != sizeof testdata / sizeof *testdata; ++i)
      check (strfmon_l (actual, sizeof (actual), loc,
                        tesdata[i].format, testdata[i].value),
                        tesdata[i].format, testdata[i].expect);

Martin