Message ID | 201901060633.AA04157@tamuki.linet.gr.jp |
---|---|
State | Committed |
Headers |
Received: (qmail 103254 invoked by alias); 6 Jan 2019 06:36:17 -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 103236 invoked by uid 89); 6 Jan 2019 06:36:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=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=2122, H*UA:Version X-HELO: mail.linet.jp Message-Id: <201901060633.AA04157@tamuki.linet.gr.jp> From: TAMUKI Shoichi <tamuki@linet.gr.jp> Date: Sun, 06 Jan 2019 15:33:37 +0900 To: libc-alpha@sourceware.org Subject: [PATCH v5 1/5] strftime: Add missing uses of L_ macro, etc. [BZ #23758] In-Reply-To: <201901060628.AA04156@tamuki.linet.gr.jp> References: <201901060628.AA04156@tamuki.linet.gr.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii |
Commit Message
TAMUKI Shoichi
Jan. 6, 2019, 6:33 a.m. UTC
At first, make an unrelated changes for the consistency. ChangeLog: [BZ #23758] * time/strftime_l.c (__strftime_internal): Add missing uses of L_ macro, also add a missing space after the cast of _NL_CURRENT. --- time/strftime_l.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Comments
Hello Tamuki Shoichi, Thank you for the next version of your patches. 1. Please remove any reference to [BZ #23758] from this patch because it is not related with the bug. The changes are minor and not visible for the users therefore they don't need any Bugzilla report, documentation, etc. 2. Regarding the subject of this email, which is also the first line of the commit message, I would write something like this: "strftime: Consequently use the "L_" macro with character literals." As always, I am not a native speaker so other people may provide better hints. 6.01.2019 07:33 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote: > > At first, make an unrelated changes for the consistency. If it is the part of the commit message then something like: "Make unrelated changes for the consistency." (The core problem is that "an" is incorrect for plural numbers.) > > ChangeLog: > > [BZ #23758] > * time/strftime_l.c (__strftime_internal): Add missing uses of L_ > macro, also add a missing space after the cast of _NL_CURRENT. Good but again, Bugzilla mention should be removed and "missing uses" seems incorrect to me, "Add "L_" macros" or "Use "L_" macros" is sufficient and seems correct to me. I cut the patch body here because it is correct and trivial, also I think it does not introduce any changes in the binary code but it's good because it improves the readability of the source code. Therefore I think it is OK to push it now with the changes above. But due to the freeze period I'd like to hear "OK" from Siddhesh therefore I'm adding CC: to him. Regards, Rafal
On 09/01/19 3:32 PM, Rafal Luzynski wrote: > Hello Tamuki Shoichi, > > Thank you for the next version of your patches. > > 1. Please remove any reference to [BZ #23758] from this patch > because it is not related with the bug. The changes are minor > and not visible for the users therefore they don't need any > Bugzilla report, documentation, etc. > > 2. Regarding the subject of this email, which is also the first > line of the commit message, I would write something like this: > > "strftime: Consequently use the "L_" macro with character literals." > > As always, I am not a native speaker so other people may provide > better hints. > > 6.01.2019 07:33 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote: >> >> At first, make an unrelated changes for the consistency. > > If it is the part of the commit message then something like: > > "Make unrelated changes for the consistency." > > (The core problem is that "an" is incorrect for plural numbers.) > >> >> ChangeLog: >> >> [BZ #23758] >> * time/strftime_l.c (__strftime_internal): Add missing uses of L_ >> macro, also add a missing space after the cast of _NL_CURRENT. > > Good but again, Bugzilla mention should be removed and "missing > uses" seems incorrect to me, "Add "L_" macros" or "Use "L_" macros" > is sufficient and seems correct to me. > > I cut the patch body here because it is correct and trivial, also > I think it does not introduce any changes in the binary code but > it's good because it improves the readability of the source code. > Therefore I think it is OK to push it now with the changes above. > But due to the freeze period I'd like to hear "OK" from Siddhesh > therefore I'm adding CC: to him. OK. Siddhesh
9.01.2019 11:21 Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > > On 09/01/19 3:32 PM, Rafal Luzynski wrote: > > [...] > > Therefore I think it is OK to push it now with the changes above. > > But due to the freeze period I'd like to hear "OK" from Siddhesh > > therefore I'm adding CC: to him. > > OK. This means: Tamuki Shoichi, please push this patch (only this first one) with the changes as suggested above. [1] Do you need an assistance for this? Regards, Rafal [1] https://sourceware.org/ml/libc-alpha/2019-01/msg00193.html
Hello Rafal, From: Rafal Luzynski <digitalfreak@lingonborough.com> Subject: Re: [PATCH v5 1/5] strftime: Add missing uses of L_ macro, etc. [BZ #23758] Date: Wed, 9 Jan 2019 11:02:25 +0100 (CET) > 1. Please remove any reference to [BZ #23758] from this patch > because it is not related with the bug. The changes are minor > and not visible for the users therefore they don't need any > Bugzilla report, documentation, etc. OK. I will do that as an independent patch. > 2. Regarding the subject of this email, which is also the first > line of the commit message, I would write something like this: > > "strftime: Consequently use the "L_" macro with character literals." > > As always, I am not a native speaker so other people may provide > better hints. Thank you. > > At first, make an unrelated changes for the consistency. > > If it is the part of the commit message then something like: > > "Make unrelated changes for the consistency." > > (The core problem is that "an" is incorrect for plural numbers.) Thank you. Also, "At first" is also incorrect, should be "First" (or "First of all"). But I will not add any of them this time because of an independent patch. > > ChangeLog: > > > > [BZ #23758] > > * time/strftime_l.c (__strftime_internal): Add missing uses of L_ > > macro, also add a missing space after the cast of _NL_CURRENT. > > Good but again, Bugzilla mention should be removed and "missing > uses" seems incorrect to me, "Add "L_" macros" or "Use "L_" macros" > is sufficient and seems correct to me. OK. I will correct in the next patch. Regards, TAMUKI Shoichi
Hello Rafal, From: Rafal Luzynski <digitalfreak@lingonborough.com> Subject: Re: [PATCH v5 1/5] strftime: Add missing uses of L_ macro, etc. [BZ #23758] Date: Wed, 9 Jan 2019 23:58:01 +0100 (CET) > > > Therefore I think it is OK to push it now with the changes above. > > > But due to the freeze period I'd like to hear "OK" from Siddhesh > > > therefore I'm adding CC: to him. > > > > OK. > > This means: Tamuki Shoichi, please push this patch (only this first one) > with the changes as suggested above. [1] > > Do you need an assistance for this? I have sent the latest patches reflecting your suggest. If it does not matter, I will push the patch (first, only the first one). This is my first time to push to publish the changes upstream. Please let me know if there is my sourceware username "tamuki". Regards, TAMUKI Shoichi
11.01.2019 05:54 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote: > > Hello Rafal, > > From: Rafal Luzynski <digitalfreak@lingonborough.com> > Subject: Re: [PATCH v5 1/5] strftime: Add missing uses of L_ macro, etc. > [BZ #23758] > Date: Wed, 9 Jan 2019 23:58:01 +0100 (CET) > > > [...] > > Do you need an assistance for this? > > I have sent the latest patches reflecting your suggest. > > If it does not matter, I will push the patch (first, only the first > one). Yes, only the first one, with unrelated changes. > This is my first time to push to publish the changes upstream. > Please let me know if there is my sourceware username "tamuki". I am unable to verify this because I don't have any access to the administrative resources of sourceware. Other people should do it. To other people: is there anybody able to assist Tamuki Shoichi with his first commit to the main repo? I'm afraid I am unable to do it today. Regards, Rafal
On 11/01/19 10:24 AM, TAMUKI Shoichi wrote: > If it does not matter, I will push the patch (first, only the first > one). This is my first time to push to publish the changes upstream. > Please let me know if there is my sourceware username "tamuki". Have you requested an account[1]? If yes then you should have received confirmation and access details. If not, Rafal should be able to commit for you. Siddhesh [1] https://sourceware.org/cgi-bin/pdw/ps_form.cgi
diff --git a/time/strftime_l.c b/time/strftime_l.c index 6919749c630..7ba4179de3e 100644 --- a/time/strftime_l.c +++ b/time/strftime_l.c @@ -820,7 +820,7 @@ __strftime_internal (CHAR_T *s, size_t maxsize, const CHAR_T *format, if (modifier == L_('O')) goto bad_format; #ifdef _NL_CURRENT - if (! (modifier == 'E' + if (! (modifier == L_('E') && (*(subfmt = (const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(ERA_D_T_FMT))) @@ -917,7 +917,7 @@ __strftime_internal (CHAR_T *s, size_t maxsize, const CHAR_T *format, #ifdef _NL_CURRENT if (! (modifier == L_('E') && (*(subfmt = - (const CHAR_T *)_NL_CURRENT (LC_TIME, NLW(ERA_D_FMT))) + (const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(ERA_D_FMT))) != L_('\0')))) subfmt = (const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(D_FMT)); goto subformat; @@ -1262,7 +1262,7 @@ __strftime_internal (CHAR_T *s, size_t maxsize, const CHAR_T *format, DO_NUMBER (1, tp->tm_wday); case L_('Y'): - if (modifier == 'E') + if (modifier == L_('E')) { #if HAVE_STRUCT_ERA_ENTRY struct era_entry *era = _nl_get_era_entry (tp HELPER_LOCALE_ARG);