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

[Rafal Luzynski <digitalfreak at lingonborough dot com>]

Carlos,

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.

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?

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

> diff --git a/localedata/Makefile b/localedata/Makefile
> index d51064adec..2e6e0dcb2a 100644
> --- a/localedata/Makefile
> +++ b/localedata/Makefile
> @@ -34,7 +34,9 @@ vpath %.h tests-mbwc
>  
>  
>  test-srcs := collate-test xfrm-test tst-fmon tst-rpmatch tst-trans \
> -	     tst-ctype tst-langinfo tst-langinfo-static tst-numeric
> +	     tst-ctype tst-langinfo-newlocale tst-langinfo-setlocale \
> +	     tst-langinfo-newlocale-static tst-langinfo-setlocale-static \
> +	     tst-numeric

OK, you remove tst-langinfo and tst-langinfo-static and add
tst-langinfo-newlocale, tst-langinfo-setlocale, tst-langinfo-newlocale-static,
and tst-langinfo-setlocale-static instead.

> -tests-static += tst-langinfo-static
> +tests-static += tst-langinfo-newlocale-static tst-langinfo-setlocale-static

OK, consequently tst-langinfo-static replaced with
tst-langinfo-newlocale-static and tst-langinfo-setlocale-static.
Where is tst-langinfo replaced with tst-langinfo-newlocale and
tst-langinfo-setlocale?  I guess it happens somewhere automagically.

> -		 $(objpfx)tst-langinfo.out $(objpfx)tst-langinfo-static.out \
> +		 $(objpfx)tst-langinfo-newlocale.out \
> +		 $(objpfx)tst-langinfo-setlocale.out \
> +		 $(objpfx)tst-langinfo-newlocale-static.out \
> +		 $(objpfx)tst-langinfo-setlocale-static.out \

OK, consequently the test results file names changed and the same in more
places in Makefile, so I will just skip it.

> 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?

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

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


> OK to commit?

Again, my review is very incomplete so I cannot guarantee and I cannot
give my "Reviewed-by" tag.

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.

Regards,

Rafal