This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]