git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Mar 14, 2014, 02:16 UTC
Message-ID
<CAPig+cRF_eQiGugR8TSks5ki375y-5wiQ7HWKyKRudJ5apd4cg@mail.gmail.com>
In-Reply-To
<1394742059-7300-1-git-send-email-zhaox383@umn.edu>
Thanks for the resubmission. Comments below.
On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao <zhaox383@umn.edu> wrote:
> Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven

It's a good idea to let reviewers know that this is attempt 2. Do so by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option for "git format-email" can help.

When your patch is applied via "git am", text inside [...] gets stripped automatically. The "GSoC" tells email readers what this submission is about, but isn't relevant to the actual commit message. It should be placed inside [...]. For instance: [PATCH/GSoC v2].

Show 8 quoted lines
> Signed-off-by: Yao Zhao <zhaox383@umn.edu>
> ---
> GSoC_MicroProject_#8
>
> Hello Eric,
>
> Thanks for reviewing my code. I implemented table-driven method this time and correct the style
> problems indicated in review.

Explaining what you changed since the last version is indeed good etiquette. Thanks. For bonus points, also provide a link to the previous version, like this [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243919
Show 12 quoted lines
>  branch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..6451c99 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -49,10 +49,43 @@ static int should_setup_rebase(const char *origin)
>
>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
>  {
> +
Unnecessary insertion of blank line.
Show 5 quoted lines
>         const char *shortname = remote + 11;
>         int remote_is_branch = starts_with(remote, "refs/heads/");
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
> +       int size=8, i;
Style: whitespace: size = 8

You can use ARRAY_SIZE(print_list) to avoid hardcoding 8 (and then you don't need the variable 'size').

> +       typedef struct PRINT_LIST {
> +               const char *print_str;
> +               const char *arg2;
> +               //arg1 is always local, so I only add arg2 and arg3 in struct

This commentary should be placed below the "---" under your sign-off (or dropped altogether since it's pretty obvious).

Also, in this project avoid //-style comments.
> +               const char *arg3;
> +               int b_rebasing;
> +         int b_remote_is_branch;

Strange indentation. Use tabs for indentation, and set your editor so tabs have width 8.

> +               int b_origin;
> +       } PRINT_LIST;
Read below for some commentary about b_rebasing, b_remote_is_branch, b_origin.
Show 22 quoted lines
> +       PRINT_LIST print_list[] = {
> +               {.print_str = _("Branch %s set up to track remote branch %s from %s by rebasing."),
> +                               .arg2 = shortname, .arg3 = origin,
> +                                        .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 1},
> +         {.print_str = _("Branch %s set up to track remote branch %s from %s."),
> +                               .arg2 = shortname, .arg3 = origin,
> +                                        .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 1},
> +    {.print_str = _("Branch %s set up to track local branch %s by rebasing."),
> +                               .arg2 = shortname, .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 0},
> +               {.print_str = _("Branch %s set up to track local branch %s."),
> +                               .arg2 = shortname, .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 0},
> +               {.print_str = _("Branch %s set up to track remote ref %s by rebasing."),
> +                               .arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 1},
> +               {.print_str = _("Branch %s set up to track remote ref %s."),
> +                               .arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 1},
> +               {.print_str = _("Branch %s set up to track local ref %s by rebasing."),
> +                               .arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 0},
> +               {.print_str = _("Branch %s set up to track local ref %s."),
> +                               .arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 0},
> +};
> I am confused here: I use struct initializer and I am not sure if it's ok
> because it is only supported by ANSI
This commentary should be placed below "---" after your sign-off.

Indeed, you want to avoid named field initializers in this project and instead use positional initializers.

Translatable strings in an initializer should be wrapped with N_() instead of _(). You will still need to use _() later on when you reference the string from the table. See section 4.7 [2] of the GNU gettext manual for details.

[2]: http://www.gnu.org/software/gettext/manual/gettext.html#Special-cases
Show 30 quoted lines
>         if (remote_is_branch
>             && !strcmp(local, shortname)
> @@ -75,31 +108,26 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>                 git_config_set(key.buf, "true");
>         }
>         strbuf_release(&key);
> -
>         if (flag & BRANCH_CONFIG_VERBOSE) {
> -               if (remote_is_branch && origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track remote branch %s from %s by rebasing.") :
> -                                 _("Branch %s set up to track remote branch %s from %s."),
> -                                 local, shortname, origin);
> -               else if (remote_is_branch && !origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track local branch %s by rebasing.") :
> -                                 _("Branch %s set up to track local branch %s."),
> -                                 local, shortname);
> -               else if (!remote_is_branch && origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track remote ref %s by rebasing.") :
> -                                 _("Branch %s set up to track remote ref %s."),
> -                                 local, remote);
> -               else if (!remote_is_branch && !origin)
> -                       printf_ln(rebasing ?
> -                                 _("Branch %s set up to track local ref %s by rebasing.") :
> -                                 _("Branch %s set up to track local ref %s."),
> -                                 local, remote);
> -               else
> +               for (i=0;i<size;i++)
Style: whitespace: for (i = 0; i < size; i++)
> +               {
> +                       if (print_list[i].b_rebasing == (rebasing? 1 : 0) &&
> +                                                               print_list[i].b_remote_is_branch == (remote_is_branch? 1 : 0) &&
> +                                                               print_list[i].b_origin == (origin? 1 : 0))
Style: whitespace: (x ? 1 : 0)

Assigning &print_list[i] to a variable would allow you to reduce the noise of this expression a bit.

On this project it is more idiomatic to say !!rebasing, !!remote_is_branch, !!origin to constrain these values to 0 or 1.

An alternate approach might be to use a multi-dimensional array, where the boolean values of rebasing, remote_is_branch, and origin are keys into the array. This would allow you to pick out the correct PRINT_LIST entry directly (no looping), thus eliminating the need for those b_rebasing, b_remote_is_branch, and b_origin members.

> +                       {
> +                               if (print_list[i].arg3 == NULL)
> +                                       printf_ln (print_list[i].print_str, local, print_list[i].arg2);
Style: whitespace: printf_ln(...)
Reminder: wrap _() around print_list[i].print_str
> +                               else
> +                                       printf_ln (print_list[i].print_str, local,
> +                                                       print_list[i].arg2, print_list[i].arg3);

This logic can be simplified. Hint: It is not an error to pass more arguments than there are %s's in the format string.

Show 8 quoted lines
> +
> +                               break;
> +                       }
> +               }
> +               if (i == size)
>                         die("BUG: impossible combination of %d and %p",
>                             remote_is_branch, origin);
> +
Unnecessary insertion of blank line.
Show 5 quoted lines
>         }
>  }
>
> --
> 1.8.3.2
Previous: Yao ZhaoNext: Junio C Hamano
Message 4 of 13 in “SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach”
  1. SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approachYao Zhao, Mar 12, 2014
  2. Eric SunshineMar 12, 2014
  3. GSoC Change multiple if-else statements to be table-drivenYao Zhao, Mar 13, 2014
  4. Eric SunshineMar 14, 2014
  5. Junio C HamanoMar 14, 2014
  6. Eric SunshineMar 17, 2014
  7. Junio C HamanoMar 17, 2014
  8. Michael HaggertyMar 17, 2014
  9. Junio C HamanoMar 17, 2014
  10. Eric SunshineMar 17, 2014
  11. Quint GuvernatorMar 17, 2014
  12. branch.c: turn nested if-else logic to table-drivenYao Zhao, Mar 16, 2014
  13. Eric SunshineMar 17, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.