Re: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[Carlos O'Donell <carlos at redhat dot com>]

On 05/23/2018 07:55 PM, Rafal Luzynski wrote:
> This is a very incomplete review.  So far I can only confirm that your
> patch compiles and adds one more XFAIL to the test results.

Thanks for that review.

I've committed what I posted.

Comments follow.

> 21.05.2018 20:14 Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> [...] In addition we add CURRENCY_SYMBOL test coverage
>> which was the original problem reported in the related gcc/C++ PR.
> 
> Would you please mention it in the commit message?

I tried to keep the commit message simple.

To meet your requirement I have added the gcc/C++ PR linked into
bug 23164 e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85732

This way the bugzilla can be kept up to date with all the linked-in
bugs from gcc/C++ PR that relate to this issue.

>> There is a glibc optimization which allows for locale categories
>> to be removed during static compilation. There have been various
>> [...]
> 
> I think this is just a nitpick but as far as I can see most commit
> messages contain a double space between the sentences.  The same
> applies to your whole commit message.

Thanks. I thought I had fixed this, but apparently I missed this.

Yes, we should follow GNU formatting and use 2 spaces after a period.


>> diff --git a/localedata/tst-langinfo-newlocale-static.c
>> b/localedata/tst-langinfo-newlocale-static.c
>> new file mode 100644
>> index 0000000000..8097ecd96f
>> --- /dev/null
>> +++ b/localedata/tst-langinfo-newlocale-static.c
>> @@ -0,0 +1 @@
>> +#include <tst-langinfo-newlocale.c>
> 
> Maybe I'm wrong but instead of creating a new file whose only line is
> "#include <another-file.c>" wouldn't it be better to generate the static
> binaries from the same source file(s) but just with different build
> options?

The problem is that the glibc build infrastructure ties the build options
to the file name.

e.g.
test-foo-CFLAGS = -static

Therefore you can only have either shared or static on the same source
file, which is why you see a common pattern in glibc which is to include
the shared source file in a "static" variant.

If we had reliable support for symlinks in VCS then we could use that too,
but we don't.

>> diff --git a/localedata/tst-langinfo-setlocale-static.c
>> b/localedata/tst-langinfo-setlocale-static.c
>> new file mode 100644
>> index 0000000000..055d1325c4
>> --- /dev/null
>> +++ b/localedata/tst-langinfo-setlocale-static.c
> 
> Same here.

Same answer.

> diff --git a/localedata/tst-langinfo.sh b/localedata/tst-langinfo.sh
> index d6787ca369..400ea6d36c 100755
> --- a/localedata/tst-langinfo.sh
> +++ b/localedata/tst-langinfo.sh
> @@ -157,6 +157,7 @@ en_US.ISO-8859-1     RADIXCHAR   .
>  en_US.ISO-8859-1     THOUSEP     ,
>  en_US.ISO-8859-1     YESEXPR     ^[+1yY]
>  en_US.ISO-8859-1     NOEXPR      ^[-0nN]
> +en_US.UTF-8	     CURRENCY_SYMBOL	$
> 
> OK, correct currency symbol for USA, and also correct for Germany, France,
> and Japan below (skipped here).

Thanks for checking! 

>> OK to commit?
> 
> Again, my review is very incomplete so I cannot guarantee and I cannot
> give my "Reviewed-by" tag.

Thanks, that's OK.

> That said, I don't object so unless anybody else objects it is OK.
> Feel free to take my nitpicks into account or reject them, they
> are not strong objections.

Thank you again for the review!

In the future may I ask that you TO me directly on a review.
It's hard for me to filter reviews of my patches that only respond to
the list?

-- 
Cheers,
Carlos.