[GSOC][PATCH] Fixed an issue which contained extra unnecessary code

[GSOC][PATCH] Fixed an issue which contained extra unnecessary code

[sushmaunnibhavi]

From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001
From: sushmaunnibhavi <sushmaunnibhavi@gmail.com>
Date: Sun, 10 Mar 2019 19:37:33 +0530
Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
Signed-off-by: Sushma Unnibhavi <sushmaunnibhavi425@gmail.com>
---
Since '\n' and '\0' are char_len==1,it is not necessary to check if the char_len<=1.
 compat/regex/regexec.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
index 1b5d89fd5e..df62597531 100644
--- a/compat/regex/regexec.c
+++ b/compat/regex/regexec.c
@@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,
   char_len = re_string_char_size_at (input, str_idx);
   if (node->type == OP_PERIOD)
     {
-      if (char_len <= 1)
-	return 0;
-      /* FIXME: I don't think this if is needed, as both '\n'
-	 and '\0' are char_len == 1.  */
-      /* '.' accepts any one character except the following two cases.  */
       if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
 	   re_string_byte_at (input, str_idx) == '\n') ||
 	  ((dfa->syntax & RE_DOT_NOT_NULL) &&
-- 
2.17.1

Re: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code

[Christian Couder]

On Sun, Mar 10, 2019 at 4:30 PM sushmaunnibhavi
<sushmaunnibhavi425@gmail.com> wrote:
>
> From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001
> From: sushmaunnibhavi <sushmaunnibhavi@gmail.com>
> Date: Sun, 10 Mar 2019 19:37:33 +0530
> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
> Signed-off-by: Sushma Unnibhavi <sushmaunnibhavi425@gmail.com>
> ---
> Since '\n' and '\0' are char_len==1,it is not necessary to check if the char_len<=1.
>  compat/regex/regexec.c | 5 -----
>  1 file changed, 5 deletions(-)

It doesn't look like the patch is formatted correctly. I think that
the explanation line ("Since '\n' and '\0' are...") should be above
the line that contains your "Signed-off-by: ..." and there should be a
blank line between those two lines.

Also we ask for an author name in the "From: ..." header that looks
like "Firstname Lastname". A simple way to do that would be to make it
match the name in your "Signed-off-by: ...".

> diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
> index 1b5d89fd5e..df62597531 100644
> --- a/compat/regex/regexec.c
> +++ b/compat/regex/regexec.c
> @@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,
>    char_len = re_string_char_size_at (input, str_idx);
>    if (node->type == OP_PERIOD)
>      {
> -      if (char_len <= 1)
> -       return 0;
> -      /* FIXME: I don't think this if is needed, as both '\n'
> -        and '\0' are char_len == 1.  */
> -      /* '.' accepts any one character except the following two cases.  */
>        if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
>            re_string_byte_at (input, str_idx) == '\n') ||
>           ((dfa->syntax & RE_DOT_NOT_NULL) &&

The code looks like:

 char_len = re_string_char_size_at (input, str_idx);
 if (node->type == OP_PERIOD)
    {
      if (char_len <= 1)
    return 0;
      /* FIXME: I don't think this if is needed, as both '\n'
     and '\0' are char_len == 1.  */
      /* '.' accepts any one character except the following two cases.  */
      if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
       re_string_byte_at (input, str_idx) == '\n') ||
      ((dfa->syntax & RE_DOT_NOT_NULL) &&
       re_string_byte_at (input, str_idx) == '\0'))
    return 0;
      return char_len;
    }

If the byte at re_string_byte_at (input, str_idx) is indeed '\n' or
'\0', then yeah probably char_len == 1 so the current code should
return 0 just before the code below the FIXME comment is reached. So
in this case the 2 checks below the FIXME comment are useless because
they check re_string_byte_at (input, str_idx) == '\n') and
re_string_byte_at (input, str_idx) == '\0' which cannot happen.

So I would say that the right fix would be to remove those 2 checks,
not to remove the if (char_len <= 1) check above the FIXME comment.

Also note that I used "probably" when I wrote "then yeah probably
char_len == 1", because I think it is worth checking what
re_string_char_size_at() actually does before being too much
confident...

Re: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code

[Thomas Gummerer]

> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code

Commit messages (and titles) should always be in the imperative mood.
The title in particular should be a short description of what the
patch is doing, and should give meaningful information to people
reading the output of 'git log --oneline'.  Also we usually prefix the
title with the area of the code we're touching.

The title above is very generic, and thus not very useful to future
readers.  Something like

    compat/regex: remove unnecessary if

would give future readers a bit more context.

On 03/10, sushmaunnibhavi wrote:
> From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001
> From: sushmaunnibhavi <sushmaunnibhavi@gmail.com>
> Date: Sun, 10 Mar 2019 19:37:33 +0530
> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code

The four lines above don't need to be included here, as they are
already defined by the email headers.

The one line that may be useful here is "From: sushmaunnibhavi
<sushmaunnibhavi@gmail.com>".  That should be your full name, and
match the name in the Signed-off-by line.  Usually people just set
that up in the "From" header in the email, but if you are unable to
configure your mailer like that, including a "From: Sushma Unnibhavi
<sushmaunnibhavi425@gmail.com>" before the rest of the commit message
also works.

> Signed-off-by: Sushma Unnibhavi <sushmaunnibhavi425@gmail.com>
> ---
> Since '\n' and '\0' are char_len==1,it is not necessary to check if the char_len<=1.

This explanation of why this is a good patch should be included in the
commit message before your Signed-off-by.  If we want to apply this
change (which I don't think we want for the reasons stated below), I
think this needs a bit of a deeper analysis in the commit message, to
convince readers this is the correct thing to do.

Also please wrap commit messages at 72 characters.

>  compat/regex/regexec.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c

This file in compat/ was directly imported from upstream gawk.  We
generally don't patch this type of imported file, to make updates from
upstream easier, unless there is an actual fix from upstream that
needs to be fixed that's not going to be fixed upstream.

As this change at best removes a redundant 'if' (I can't comment on the
correctness, as I'm not familar with this code), so it is not worth
changing this file in our codebase.

> index 1b5d89fd5e..df62597531 100644
> --- a/compat/regex/regexec.c
> +++ b/compat/regex/regexec.c
> @@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,
>    char_len = re_string_char_size_at (input, str_idx);
>    if (node->type == OP_PERIOD)
>      {
> -      if (char_len <= 1)
> -	return 0;
> -      /* FIXME: I don't think this if is needed, as both '\n'
> -	 and '\0' are char_len == 1.  */
> -      /* '.' accepts any one character except the following two cases.  */
>        if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
>  	   re_string_byte_at (input, str_idx) == '\n') ||
>  	  ((dfa->syntax & RE_DOT_NOT_NULL) &&
> -- 
> 2.17.1
>