[v6] strftime: Consequently use the "L_" macro with character literals

Message ID 201901110446.AA04172@tamuki.linet.gr.jp
State New
Headers show
Series
  • [v6] strftime: Consequently use the "L_" macro with character literals
Related show

Commit Message

TAMUKI Shoichi Jan. 11, 2019, 4:46 a.m.
Make unrelated changes for the consistency.

ChangeLog:

	* time/strftime_l.c (__strftime_internal): Use "L_" macros, also add a
	missing space after the cast of "_NL_CURRENT".
---
 time/strftime_l.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Rafal Luzynski Jan. 11, 2019, 12:01 p.m. | #1
Hello,

Please add "." (a dot, a full stop) at the end of the first line
of the commit message which is also the subject of this email.
So it should be:

"strftime: Consequently use the "L_" macro with character literals."

11.01.2019 05:46 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote:
> 
> Make unrelated changes for the consistency.

I think we can remove this line.  First, the word "unrelated" seems
unclear to me because it suggests we are in some context (unrelated
with what?)  Additionally, it does not give any new information.
It is OK if the commit message consists of only one line if the patch
is trivial and the first line explains everything.

> 
> ChangeLog:
> 
> 	* time/strftime_l.c (__strftime_internal): Use "L_" macros, also add a
> 	missing space after the cast of "_NL_CURRENT".

Yes, this is perfect and it is correct to put this in the commit
message.  Also, please remember that you should also put these two
lines in the ChangeLog file itself and only then push your change.
See how other ChangeLog entries look and follow the convention.

> ---
>  time/strftime_l.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> [...]

I don't comment the rest of your patch because I reviewed it before
and it looks correct.

Please apply the changes I suggest and commit and push your patch
to the main repository.

Regards,

Rafal
Andreas Schwab Jan. 11, 2019, 1 p.m. | #2
On Jan 11 2019, Rafal Luzynski <digitalfreak@lingonborough.com> wrote:

> Please add "." (a dot, a full stop) at the end of the first line
> of the commit message which is also the subject of this email.

A subject line should not end with a period.

Andreas.
Florian Weimer Jan. 11, 2019, 1:56 p.m. | #3
* Andreas Schwab:

> On Jan 11 2019, Rafal Luzynski <digitalfreak@lingonborough.com> wrote:
>
>> Please add "." (a dot, a full stop) at the end of the first line
>> of the commit message which is also the subject of this email.
>
> A subject line should not end with a period.

We don't have a firm rule here.  Items in ChangeLog entries should end
with periods, but that doesn't automatically apply to commit subject
lines.

Thanks,
Florian
Rafal Luzynski Jan. 11, 2019, 11:03 p.m. | #4
I understood Siddhesh' suggestion [1] as the request for me to push
this patch and I have pushed it now with minor changes in the commit
message.  I have given up on the requirement of the trailing dot in
the first line for now.

Regards,

Rafal


[1] https://sourceware.org/ml/libc-alpha/2019-01/msg00250.html
Rafal Luzynski Jan. 11, 2019, 11:19 p.m. | #5
11.01.2019 14:56 Florian Weimer <fweimer@redhat.com> wrote:
> 
> * Andreas Schwab:
> 
> > On Jan 11 2019, Rafal Luzynski <digitalfreak@lingonborough.com> wrote:
> >
> >> Please add "." (a dot, a full stop) at the end of the first line
> >> of the commit message which is also the subject of this email.
> >
> > A subject line should not end with a period.
> 
> We don't have a firm rule here.  [...]

I don't remember where I saw that rule.  All I can find now is that
examples in the Contribution Checklist [1] do end with a period.
I will update here if I find anything more.

I agree that a subject line should not end with a period but no
matter how weird it looks I always add it for the sake of the commit
message format.

Regards,

Rafal


[1]
https://sourceware.org/glibc/wiki/Contribution%20checklist#Contribution_Email_Subject_Line
TAMUKI Shoichi Jan. 12, 2019, 12:19 p.m. | #6
Hello Rafal,

From: Rafal Luzynski <digitalfreak@lingonborough.com>
Subject: Re: [PATCH v6] strftime: Consequently use the "L_" macro with character literals
Date: Fri, 11 Jan 2019 13:01:54 +0100 (CET)

> > Make unrelated changes for the consistency.
> 
> I think we can remove this line.  First, the word "unrelated" seems
> unclear to me because it suggests we are in some context (unrelated
> with what?)  Additionally, it does not give any new information.
> It is OK if the commit message consists of only one line if the patch
> is trivial and the first line explains everything.

I have no particular preference about this, so I agree.

Regards,
TAMUKI Shoichi
TAMUKI Shoichi Jan. 12, 2019, 1:04 p.m. | #7
Hello Rafal,

From: Rafal Luzynski <digitalfreak@lingonborough.com>
Subject: Re: [PATCH v6] strftime: Consequently use the "L_" macro with character literals
Date: Sat, 12 Jan 2019 00:03:57 +0100 (CET)

> I understood Siddhesh' suggestion [1] as the request for me to push
> this patch and I have pushed it now with minor changes in the commit
> message.  I have given up on the requirement of the trailing dot in
> the first line for now.

Thank you for pushing my patch with minor changes in the commit
message.

Yes, I prefer not to end the first line with a period, because it is a
title and titles do not end with a period.

There is not a firm rule in Glibc Wiki, indeed.  On the other hand, it
commonly seems that the summary line of commit message should not end
with a period.  (Googling "git commit summary period".)

Regards,
TAMUKI Shoichi

Patch

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