| Message ID | 416728387.157880.1527119759246@poczta.nazwa.pl |
|---|---|
| State | Not applicable |
| Headers |
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: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> 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 <digitalfreak@lingonborough.com> Reply-To: Rafal Luzynski <digitalfreak@lingonborough.com> To: GNU C Library <libc-alpha@sourceware.org>, Carlos O'Donell <carlos@redhat.com> 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 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Originating-Client: com.openexchange.ox.gui.dhtml |
Commit Message
Rafal Luzynski
May 23, 2018, 11:55 p.m. UTC
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. > 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
Comments
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?
4.07.2018 21:35 Carlos O'Donell <carlos@redhat.com> wrote: > > 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. Thank you, good to see this commit pushed. Shouldn't you close the bug report as well, and mark the Target Milestone 2.28? > 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. I like what you have put in the ChangeLog and I thought that every commit message should contain the ChangeLog entry copied. Of course it was correct to remove ChangeLog diff in libc-alpha post. Never mind, maybe I'm wrong and even if not it is probably impossible to fix the commit comment now. > 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. That's more than I expected but great, thank you. [ cut the part where I just totally agree ] > [...] > 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? That's what I usually do. Didn't I this time? I'm sorry about that. I don't understand why I did not, most probably my mistake. If I had any good reason then I don't remember it now, it's been a while ago. Regards, Rafal
On 07/04/2018 04:28 PM, Rafal Luzynski wrote: > 4.07.2018 21:35 Carlos O'Donell <carlos@redhat.com> wrote: >> >> 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. > > Thank you, good to see this commit pushed. Shouldn't you close the > bug report as well, and mark the Target Milestone 2.28? No, the bug is not fixed. I just added the regression tests that prove the bug is not fixed. Someone still has to do the work to fix the bug. So sadly the bug is still there :-( ... but having a reproducible test case is half the battle. >> [...] >> 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? > > That's what I usually do. Didn't I this time? I'm sorry about that. > I don't understand why I did not, most probably my mistake. If I had > any good reason then I don't remember it now, it's been a while ago. Thanks. No worries. I found the review by going back to the list and cross checking for any replies.
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).