| Message ID | 20190117130615.18732-29-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert diff opt parser to parse_options() | expand |
Hi Duy, On Thu, 17 Jan 2019, Nguyễn Thái Ngọc Duy wrote: > +static int diff_opt_break_rewrites(const struct option *opt, > + const char *arg, int unset) > +{ > + int *break_opt = opt->value; > + int opt1, opt2; > + > + BUG_ON_OPT_NEG(unset); > + if (!arg) > + arg = ""; > + opt1 = parse_rename_score(&arg); > + switch (*arg) { > + case '\0': > + opt2 = 0; > + break; > + case '/': > + arg++; > + opt2 = parse_rename_score(&arg); > + break; > + } This code snippet is anywhere in the spectrum between smart, cute, clever, hard to reason about, and difficult to validate. Granted, Git for Windows SDK's GCC v7.3.0 seems to be able to figure out (somehow...) that this does not leave `opt2` uninitialized. But Ubuntu 16.04's default GCC version (which I believe is v5.3.1) is not. And likewise, human readers have to spend way too much time thinking about this. So I would strongly suggest to save everybody and their compiler some time by squashing this in: -- snip -- t a/diff.c b/diff.c index 381259e987a5..855e6ddcb2b9 100644 --- a/diff.c +++ b/diff.c @@ -4949,16 +4949,13 @@ static int diff_opt_break_rewrites(const struct option *opt, const char *arg, int unset) { int *break_opt = opt->value; - int opt1, opt2; + int opt1, opt2 = 0; BUG_ON_OPT_NEG(unset); if (!arg) arg = ""; opt1 = parse_rename_score(&arg); switch (*arg) { - case '\0': - opt2 = 0; - break; case '/': arg++; opt2 = parse_rename_score(&arg); -- snap -- Not only is the result a lot easier to understand and to reason about, it also saves 3 lines. Ciao, Johannes P.S.: Please do not send the entire 78 "re-rolled" patches my way, should you choose to send another iteration of this unsplit patch series, but just this one. TIA > + if (*arg != 0) > + return error(_("%s expects <n>/<m> form"), opt->long_name); > + *break_opt = opt1 | (opt2 << 16); > + return 0; > +} > + > static int diff_opt_char(const struct option *opt, > const char *arg, int unset) > { > @@ -5014,6 +5039,12 @@ static void prep_parse_options(struct diff_options *options) > N_("specify the character to indicate a context instead of ' '"), > PARSE_OPT_NONEG, diff_opt_char), > > + OPT_GROUP(N_("Diff rename options")), > + OPT_CALLBACK_F('B', "break-rewrites", &options->break_opt, N_("<n>[/<m>]"), > + N_("break complete rewrite changes into pairs of delete and create"), > + PARSE_OPT_NONEG | PARSE_OPT_OPTARG, > + diff_opt_break_rewrites), > + > OPT_GROUP(N_("Diff other options")), > { OPTION_CALLBACK, 0, "output", options, N_("<file>"), > N_("Output to a specific file"), > @@ -5047,12 +5078,7 @@ int diff_opt_parse(struct diff_options *options, > return ac; > > /* renames options */ > - if (starts_with(arg, "-B") || > - skip_to_optional_arg(arg, "--break-rewrites", NULL)) { > - if ((options->break_opt = diff_scoreopt_parse(arg)) == -1) > - return error("invalid argument to -B: %s", arg+2); > - } > - else if (starts_with(arg, "-M") || > + if (starts_with(arg, "-M") || > skip_to_optional_arg(arg, "--find-renames", NULL)) { > if ((options->rename_score = diff_scoreopt_parse(arg)) == -1) > return error("invalid argument to -M: %s", arg+2); > @@ -5331,17 +5357,14 @@ int parse_rename_score(const char **cp_p) > > static int diff_scoreopt_parse(const char *opt) > { > - int opt1, opt2, cmd; > + int opt1, cmd; > > if (*opt++ != '-') > return -1; > cmd = *opt++; > if (cmd == '-') { > /* convert the long-form arguments into short-form versions */ > - if (skip_prefix(opt, "break-rewrites", &opt)) { > - if (*opt == 0 || *opt++ == '=') > - cmd = 'B'; > - } else if (skip_prefix(opt, "find-copies", &opt)) { > + if (skip_prefix(opt, "find-copies", &opt)) { > if (*opt == 0 || *opt++ == '=') > cmd = 'C'; > } else if (skip_prefix(opt, "find-renames", &opt)) { > @@ -5349,25 +5372,13 @@ static int diff_scoreopt_parse(const char *opt) > cmd = 'M'; > } > } > - if (cmd != 'M' && cmd != 'C' && cmd != 'B') > - return -1; /* that is not a -M, -C, or -B option */ > + if (cmd != 'M' && cmd != 'C') > + return -1; /* that is not a -M, or -C option */ > > opt1 = parse_rename_score(&opt); > - if (cmd != 'B') > - opt2 = 0; > - else { > - if (*opt == 0) > - opt2 = 0; > - else if (*opt != '/') > - return -1; /* we expect -B80/99 or -B80 */ > - else { > - opt++; > - opt2 = parse_rename_score(&opt); > - } > - } > if (*opt != 0) > return -1; > - return opt1 | (opt2 << 16); > + return opt1; > } > > struct diff_queue_struct diff_queued_diff; > -- > 2.20.0.482.g66447595a7 > > >
diff --git a/diff.c b/diff.c index c70de7f358..d4c378e836 100644 --- a/diff.c +++ b/diff.c @@ -4844,6 +4844,31 @@ static int parse_objfind_opt(struct diff_options *opt, const char *arg) return 1; } +static int diff_opt_break_rewrites(const struct option *opt, + const char *arg, int unset) +{ + int *break_opt = opt->value; + int opt1, opt2; + + BUG_ON_OPT_NEG(unset); + if (!arg) + arg = ""; + opt1 = parse_rename_score(&arg); + switch (*arg) { + case '\0': + opt2 = 0; + break; + case '/': + arg++; + opt2 = parse_rename_score(&arg); + break; + } + if (*arg != 0) + return error(_("%s expects <n>/<m> form"), opt->long_name); + *break_opt = opt1 | (opt2 << 16); + return 0; +} + static int diff_opt_char(const struct option *opt, const char *arg, int unset) { @@ -5014,6 +5039,12 @@ static void prep_parse_options(struct diff_options *options) N_("specify the character to indicate a context instead of ' '"), PARSE_OPT_NONEG, diff_opt_char), + OPT_GROUP(N_("Diff rename options")), + OPT_CALLBACK_F('B', "break-rewrites", &options->break_opt, N_("<n>[/<m>]"), + N_("break complete rewrite changes into pairs of delete and create"), + PARSE_OPT_NONEG | PARSE_OPT_OPTARG, + diff_opt_break_rewrites), + OPT_GROUP(N_("Diff other options")), { OPTION_CALLBACK, 0, "output", options, N_("<file>"), N_("Output to a specific file"), @@ -5047,12 +5078,7 @@ int diff_opt_parse(struct diff_options *options, return ac; /* renames options */ - if (starts_with(arg, "-B") || - skip_to_optional_arg(arg, "--break-rewrites", NULL)) { - if ((options->break_opt = diff_scoreopt_parse(arg)) == -1) - return error("invalid argument to -B: %s", arg+2); - } - else if (starts_with(arg, "-M") || + if (starts_with(arg, "-M") || skip_to_optional_arg(arg, "--find-renames", NULL)) { if ((options->rename_score = diff_scoreopt_parse(arg)) == -1) return error("invalid argument to -M: %s", arg+2); @@ -5331,17 +5357,14 @@ int parse_rename_score(const char **cp_p) static int diff_scoreopt_parse(const char *opt) { - int opt1, opt2, cmd; + int opt1, cmd; if (*opt++ != '-') return -1; cmd = *opt++; if (cmd == '-') { /* convert the long-form arguments into short-form versions */ - if (skip_prefix(opt, "break-rewrites", &opt)) { - if (*opt == 0 || *opt++ == '=') - cmd = 'B'; - } else if (skip_prefix(opt, "find-copies", &opt)) { + if (skip_prefix(opt, "find-copies", &opt)) { if (*opt == 0 || *opt++ == '=') cmd = 'C'; } else if (skip_prefix(opt, "find-renames", &opt)) { @@ -5349,25 +5372,13 @@ static int diff_scoreopt_parse(const char *opt) cmd = 'M'; } } - if (cmd != 'M' && cmd != 'C' && cmd != 'B') - return -1; /* that is not a -M, -C, or -B option */ + if (cmd != 'M' && cmd != 'C') + return -1; /* that is not a -M, or -C option */ opt1 = parse_rename_score(&opt); - if (cmd != 'B') - opt2 = 0; - else { - if (*opt == 0) - opt2 = 0; - else if (*opt != '/') - return -1; /* we expect -B80/99 or -B80 */ - else { - opt++; - opt2 = parse_rename_score(&opt); - } - } if (*opt != 0) return -1; - return opt1 | (opt2 << 16); + return opt1; } struct diff_queue_struct diff_queued_diff;
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- diff.c | 63 ++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 26 deletions(-)