From: Eric Sunshine <sunshine@sunshineco.com>
To: Yao Zhao <zhaox383@umn.edu>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] GSoC Change multiple if-else statements to be table-driven
Date: Thu, 13 Mar 2014 22:16:41 -0400 [thread overview]
Message-ID: <CAPig+cRF_eQiGugR8TSks5ki375y-5wiQ7HWKyKRudJ5apd4cg@mail.gmail.com> (raw)
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].
> 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
> 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.
> 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.
> + 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
> 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.
> +
> + break;
> + }
> + }
> + if (i == size)
> die("BUG: impossible combination of %d and %p",
> remote_is_branch, origin);
> +
Unnecessary insertion of blank line.
> }
> }
>
> --
> 1.8.3.2
next prev parent reply other threads:[~2014-03-14 2:16 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-12 3:47 [PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach Yao Zhao
2014-03-12 9:21 ` Eric Sunshine
2014-03-13 20:20 ` [PATCH] GSoC Change multiple if-else statements to be table-driven Yao Zhao
2014-03-14 2:16 ` Eric Sunshine [this message]
2014-03-14 16:54 ` Junio C Hamano
2014-03-17 6:29 ` Eric Sunshine
2014-03-17 7:31 ` Junio C Hamano
2014-03-17 14:11 ` Michael Haggerty
2014-03-17 19:12 ` Junio C Hamano
2014-03-17 19:27 ` Eric Sunshine
2014-03-17 20:39 ` Quint Guvernator
2014-03-16 6:08 ` [PATCH/GSoC_v3] branch.c: turn nested if-else logic to table-driven Yao Zhao
2014-03-17 7:54 ` Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPig+cRF_eQiGugR8TSks5ki375y-5wiQ7HWKyKRudJ5apd4cg@mail.gmail.com \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=zhaox383@umn.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).