From patchwork Wed May 23 23:55:59 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rafal Luzynski X-Patchwork-Id: 27471 Received: (qmail 65009 invoked by alias); 23 May 2018 23:56:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 64973 invoked by uid 89); 23 May 2018 23:56:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=H*x:Mailer, H*UA:Mailer, usa, USA X-HELO: ano163.rev.netart.pl X-Spam-Score: 5 Date: Thu, 24 May 2018 01:55:59 +0200 (CEST) From: Rafal Luzynski Reply-To: Rafal Luzynski To: GNU C Library , Carlos O'Donell Message-ID: <416728387.157880.1527119759246@poczta.nazwa.pl> In-Reply-To: <3b445aba-1839-edab-537a-57db9987c220@redhat.com> References: <3b445aba-1839-edab-537a-57db9987c220@redhat.com> Subject: Re: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164) MIME-Version: 1.0 X-Originating-Client: com.openexchange.ox.gui.dhtml 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 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 Maybe I'm wrong but instead of creating a new file whose only line is "#include " 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. > 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 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).