# [PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup() 9 messages from 2020-02-09 to 2020-02-10. Participants: René Scharfe, Eric Sunshine, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/52768 ## René Scharfe, 2020-02-09 15:53 Subject: [PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup() Message-ID: <11b82734-f61c-5e73-2d0c-22208c06d495@web.de> URL: https://gitlist.dev/e/11b82734-f61c-5e73-2d0c-22208c06d495%40web.de ``` Reduce code duplication. René Scharfe (4): parse-options: use COPY_ARRAY in parse_options_concat() parse-options: factor out parse_options_count() parse-options: const parse_options_concat() parameters parse-options: simplify parse_options_dup() parse-options-cb.c | 42 +++++++++++++++++------------------------- parse-options.h | 2 +- 2 files changed, 18 insertions(+), 26 deletions(-) -- 2.25.0 ``` ## René Scharfe, 2020-02-09 15:55 Subject: [PATCH 1/4] parse-options: use COPY_ARRAY in parse_options_concat() Message-ID: <074eda38-9517-26b6-5608-55ec5f473d80@web.de> URL: https://gitlist.dev/e/074eda38-9517-26b6-5608-55ec5f473d80%40web.de In-Reply-To: <11b82734-f61c-5e73-2d0c-22208c06d495@web.de> ``` Use COPY_ARRAY to copy whole arrays instead of iterating through the elements. That's shorter, simpler and bit more efficient. Signed-off-by: René Scharfe --- parse-options-cb.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/parse-options-cb.c b/parse-options-cb.c index c2062ae742..012e048856 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -188,11 +188,8 @@ struct option *parse_options_concat(struct option *a, struct option *b) b_len++; ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1)); - for (i = 0; i < a_len; i++) - ret[i] = a[i]; - for (i = 0; i < b_len; i++) - ret[a_len + i] = b[i]; - ret[a_len + b_len] = b[b_len]; /* final OPTION_END */ + COPY_ARRAY(ret, a, a_len); + COPY_ARRAY(ret + a_len, b, b_len + 1); /* + 1 for final OPTION_END */ return ret; } -- 2.25.0 ``` ## René Scharfe, 2020-02-09 15:56 Subject: [PATCH 2/4] parse-options: factor out parse_options_count() Message-ID: URL: https://gitlist.dev/e/c729aaab-68d7-9cfa-8981-97eaa72a5ebe%40web.de In-Reply-To: <11b82734-f61c-5e73-2d0c-22208c06d495@web.de> ``` Add a helper function to count the number of options (excluding the final OPT_END()) and use it to simplify parse_options_dup() and parse_options_concat(). Signed-off-by: René Scharfe --- parse-options-cb.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/parse-options-cb.c b/parse-options-cb.c index 012e048856..db6f666ef7 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -159,16 +159,20 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset) return 0; } +static size_t parse_options_count(const struct option *opt) +{ + size_t n = 0; + + for (; opt && opt->type != OPTION_END; opt++) + n++; + return n; +} + struct option *parse_options_dup(const struct option *o) { const struct option *orig = o; struct option *opts; - int nr = 0; - - while (o && o->type != OPTION_END) { - nr++; - o++; - } + size_t nr = parse_options_count(o); ALLOC_ARRAY(opts, nr + 1); COPY_ARRAY(opts, orig, nr); @@ -180,12 +184,8 @@ struct option *parse_options_dup(const struct option *o) struct option *parse_options_concat(struct option *a, struct option *b) { struct option *ret; - size_t i, a_len = 0, b_len = 0; - - for (i = 0; a[i].type != OPTION_END; i++) - a_len++; - for (i = 0; b[i].type != OPTION_END; i++) - b_len++; + size_t a_len = parse_options_count(a); + size_t b_len = parse_options_count(b); ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1)); COPY_ARRAY(ret, a, a_len); -- 2.25.0 ``` ## René Scharfe, 2020-02-09 15:57 Subject: [PATCH 3/4] parse-options: const parse_options_concat() parameters Message-ID: <2f8eeca8-bada-d67d-9c96-bf10f97a794e@web.de> URL: https://gitlist.dev/e/2f8eeca8-bada-d67d-9c96-bf10f97a794e%40web.de In-Reply-To: <11b82734-f61c-5e73-2d0c-22208c06d495@web.de> ``` Document the fact that the function doesn't modify the two option arrays passed to it by adding the keyword const to each parameter. Signed-off-by: René Scharfe --- parse-options-cb.c | 3 ++- parse-options.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/parse-options-cb.c b/parse-options-cb.c index db6f666ef7..7d56681130 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -181,7 +181,8 @@ struct option *parse_options_dup(const struct option *o) return opts; } -struct option *parse_options_concat(struct option *a, struct option *b) +struct option *parse_options_concat(const struct option *a, + const struct option *b) { struct option *ret; size_t a_len = parse_options_count(a); diff --git a/parse-options.h b/parse-options.h index fdc0c1cb97..1d60205881 100644 --- a/parse-options.h +++ b/parse-options.h @@ -281,7 +281,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx, int parse_options_end(struct parse_opt_ctx_t *ctx); struct option *parse_options_dup(const struct option *a); -struct option *parse_options_concat(struct option *a, struct option *b); +struct option *parse_options_concat(const struct option *a, const struct option *b); /*----- some often used options -----*/ int parse_opt_abbrev_cb(const struct option *, const char *, int); -- 2.25.0 ``` ## René Scharfe, 2020-02-09 15:58 Subject: [PATCH 4/4] parse-options: simplify parse_options_dup() Message-ID: <162fb36a-6dd1-c178-9032-d3f4213930a6@web.de> URL: https://gitlist.dev/e/162fb36a-6dd1-c178-9032-d3f4213930a6%40web.de In-Reply-To: <11b82734-f61c-5e73-2d0c-22208c06d495@web.de> ``` Simplify parse_options_dup() by making it a trivial wrapper of parse_options_concat() by making use of the facts that the latter duplicates its input as well and that appending an empty set is a no-op. Signed-off-by: René Scharfe --- parse-options-cb.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/parse-options-cb.c b/parse-options-cb.c index 7d56681130..a28b55be48 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -170,15 +170,9 @@ static size_t parse_options_count(const struct option *opt) struct option *parse_options_dup(const struct option *o) { - const struct option *orig = o; - struct option *opts; - size_t nr = parse_options_count(o); - - ALLOC_ARRAY(opts, nr + 1); - COPY_ARRAY(opts, orig, nr); - memset(opts + nr, 0, sizeof(*opts)); - opts[nr].type = OPTION_END; - return opts; + struct option no_options[] = { OPT_END() }; + + return parse_options_concat(o, no_options); } struct option *parse_options_concat(const struct option *a, -- 2.25.0 ``` ## Eric Sunshine, 2020-02-09 17:56 Subject: Re: [PATCH 2/4] parse-options: factor out parse_options_count() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQpzc6eZyOo9N%3D4sR3pBFza299rRn_wP0w2W7Zf5CWThg%40mail.gmail.com In-Reply-To: ``` On Sun, Feb 9, 2020 at 10:56 AM René Scharfe wrote: > Add a helper function to count the number of options (excluding the > final OPT_END()) and use it to simplify parse_options_dup() and > parse_options_concat(). > > Signed-off-by: René Scharfe > --- > diff --git a/parse-options-cb.c b/parse-options-cb.c > @@ -159,16 +159,20 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset) > struct option *parse_options_dup(const struct option *o) > { > const struct option *orig = o; > struct option *opts; > - int nr = 0; > - > - while (o && o->type != OPTION_END) { > - nr++; > - o++; > - } > + size_t nr = parse_options_count(o); > > ALLOC_ARRAY(opts, nr + 1); > COPY_ARRAY(opts, orig, nr); This could use a little more cleanup. After this change, 'o' is never again consulted or changed, and 'orig' points at the original value of 'o', which means 'o' and 'orig' have the same value now always. Therefore, the additional cleanup would be to drop the declaration and assignment of 'orig' and reference 'o' in COPY_ARRAY() rather than 'orig'. ``` ## René Scharfe, 2020-02-09 18:36 Subject: Re: [PATCH 2/4] parse-options: factor out parse_options_count() Message-ID: <4577baea-0cc6-5fd9-a1c4-699b15dae380@web.de> URL: https://gitlist.dev/e/4577baea-0cc6-5fd9-a1c4-699b15dae380%40web.de In-Reply-To: ``` Am 09.02.20 um 18:56 schrieb Eric Sunshine: > On Sun, Feb 9, 2020 at 10:56 AM René Scharfe wrote: >> Add a helper function to count the number of options (excluding the >> final OPT_END()) and use it to simplify parse_options_dup() and >> parse_options_concat(). >> >> Signed-off-by: René Scharfe >> --- >> diff --git a/parse-options-cb.c b/parse-options-cb.c >> @@ -159,16 +159,20 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset) >> struct option *parse_options_dup(const struct option *o) >> { >> const struct option *orig = o; >> struct option *opts; >> - int nr = 0; >> - >> - while (o && o->type != OPTION_END) { >> - nr++; >> - o++; >> - } >> + size_t nr = parse_options_count(o); >> >> ALLOC_ARRAY(opts, nr + 1); >> COPY_ARRAY(opts, orig, nr); > > This could use a little more cleanup. After this change, 'o' is never > again consulted or changed, and 'orig' points at the original value of > 'o', which means 'o' and 'orig' have the same value now always. > Therefore, the additional cleanup would be to drop the declaration and > assignment of 'orig' and reference 'o' in COPY_ARRAY() rather than > 'orig'. True, but that would go beyond the purpose of this patch, which is to extract a count function. While it enables the cleanup you mentioned, the latter should go into its own patch. The last one in the series takes care of it. René ``` ## Junio C Hamano, 2020-02-10 17:49 Subject: Re: [PATCH 4/4] parse-options: simplify parse_options_dup() Message-ID: URL: https://gitlist.dev/e/xmqqh7zy1gee.fsf%40gitster-ct.c.googlers.com In-Reply-To: <162fb36a-6dd1-c178-9032-d3f4213930a6@web.de> ``` René Scharfe writes: > Simplify parse_options_dup() by making it a trivial wrapper of > parse_options_concat() by making use of the facts that the latter > duplicates its input as well and that appending an empty set is a no-op. > ... > + struct option no_options[] = { OPT_END() }; > + > + return parse_options_concat(o, no_options); Can't say if this is tricky or cute, but I like it ;-) Will queue all four. Thanks. ``` ## Jeff King, 2020-02-10 22:25 Subject: Re: [PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup() Message-ID: <20200210222518.GB623231@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200210222518.GB623231%40coredump.intra.peff.net In-Reply-To: <11b82734-f61c-5e73-2d0c-22208c06d495@web.de> ``` On Sun, Feb 09, 2020 at 04:53:46PM +0100, René Scharfe wrote: > Reduce code duplication. > > René Scharfe (4): > parse-options: use COPY_ARRAY in parse_options_concat() > parse-options: factor out parse_options_count() > parse-options: const parse_options_concat() parameters > parse-options: simplify parse_options_dup() I read all 4 and they looked good to me. -Peff ```