{"thread":{"id":"52768","subject":"[PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup()","startedAt":"2020-02-09T15:54:13Z","lastAt":"2020-02-10T22:25:21Z","messageCount":9,"participants":["René Scharfe","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"391399","messageId":"11b82734-f61c-5e73-2d0c-22208c06d495@web.de","threadId":"52768","inReplyTo":null,"subject":"[PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T15:53:46Z","receivedAt":"2020-02-09T15:54:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Reduce code duplication.\n\nRené Scharfe (4):\n parse-options: use COPY_ARRAY in parse_options_concat()\n parse-options: factor out parse_options_count()\n parse-options: const parse_options_concat() parameters\n parse-options: simplify parse_options_dup()\n\n parse-options-cb.c | 42 +++++++++++++++++-------------------------\n parse-options.h | 2 +-\n 2 files changed, 18 insertions(+), 26 deletions(-)\n\n--\n2.25.0\n"},{"id":"391400","messageId":"074eda38-9517-26b6-5608-55ec5f473d80@web.de","threadId":"52768","inReplyTo":"11b82734-f61c-5e73-2d0c-22208c06d495@web.de","subject":"[PATCH 1/4] parse-options: use COPY_ARRAY in parse_options_concat()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T15:55:54Z","receivedAt":"2020-02-09T15:56:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use COPY_ARRAY to copy whole arrays instead of iterating through the\nelements. That's shorter, simpler and bit more efficient.\n\nSigned-off-by: René Scharfe \n---\n parse-options-cb.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex c2062ae742..012e048856 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -188,11 +188,8 @@ struct option *parse_options_concat(struct option *a, struct option *b)\n \t\tb_len++;\n\n \tALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));\n-\tfor (i = 0; i < a_len; i++)\n-\t\tret[i] = a[i];\n-\tfor (i = 0; i < b_len; i++)\n-\t\tret[a_len + i] = b[i];\n-\tret[a_len + b_len] = b[b_len]; /* final OPTION_END */\n+\tCOPY_ARRAY(ret, a, a_len);\n+\tCOPY_ARRAY(ret + a_len, b, b_len + 1); /* + 1 for final OPTION_END */\n\n \treturn ret;\n }\n--\n2.25.0\n"},{"id":"391401","messageId":"c729aaab-68d7-9cfa-8981-97eaa72a5ebe@web.de","threadId":"52768","inReplyTo":"11b82734-f61c-5e73-2d0c-22208c06d495@web.de","subject":"[PATCH 2/4] parse-options: factor out parse_options_count()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T15:56:47Z","receivedAt":"2020-02-09T15:56:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a helper function to count the number of options (excluding the\nfinal OPT_END()) and use it to simplify parse_options_dup() and\nparse_options_concat().\n\nSigned-off-by: René Scharfe \n---\n parse-options-cb.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 012e048856..db6f666ef7 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -159,16 +159,20 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n\n+static size_t parse_options_count(const struct option *opt)\n+{\n+\tsize_t n = 0;\n+\n+\tfor (; opt && opt->type != OPTION_END; opt++)\n+\t\tn++;\n+\treturn n;\n+}\n+\n struct option *parse_options_dup(const struct option *o)\n {\n \tconst struct option *orig = o;\n \tstruct option *opts;\n-\tint nr = 0;\n-\n-\twhile (o && o->type != OPTION_END) {\n-\t\tnr++;\n-\t\to++;\n-\t}\n+\tsize_t nr = parse_options_count(o);\n\n \tALLOC_ARRAY(opts, nr + 1);\n \tCOPY_ARRAY(opts, orig, nr);\n@@ -180,12 +184,8 @@ struct option *parse_options_dup(const struct option *o)\n struct option *parse_options_concat(struct option *a, struct option *b)\n {\n \tstruct option *ret;\n-\tsize_t i, a_len = 0, b_len = 0;\n-\n-\tfor (i = 0; a[i].type != OPTION_END; i++)\n-\t\ta_len++;\n-\tfor (i = 0; b[i].type != OPTION_END; i++)\n-\t\tb_len++;\n+\tsize_t a_len = parse_options_count(a);\n+\tsize_t b_len = parse_options_count(b);\n\n \tALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));\n \tCOPY_ARRAY(ret, a, a_len);\n--\n2.25.0\n"},{"id":"391402","messageId":"2f8eeca8-bada-d67d-9c96-bf10f97a794e@web.de","threadId":"52768","inReplyTo":"11b82734-f61c-5e73-2d0c-22208c06d495@web.de","subject":"[PATCH 3/4] parse-options: const parse_options_concat() parameters","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T15:57:56Z","receivedAt":"2020-02-09T15:58:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Document the fact that the function doesn't modify the two option arrays\npassed to it by adding the keyword const to each parameter.\n\nSigned-off-by: René Scharfe \n---\n parse-options-cb.c | 3 ++-\n parse-options.h | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex db6f666ef7..7d56681130 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -181,7 +181,8 @@ struct option *parse_options_dup(const struct option *o)\n \treturn opts;\n }\n\n-struct option *parse_options_concat(struct option *a, struct option *b)\n+struct option *parse_options_concat(const struct option *a,\n+\t\t\t\t const struct option *b)\n {\n \tstruct option *ret;\n \tsize_t a_len = parse_options_count(a);\ndiff --git a/parse-options.h b/parse-options.h\nindex fdc0c1cb97..1d60205881 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -281,7 +281,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n int parse_options_end(struct parse_opt_ctx_t *ctx);\n\n struct option *parse_options_dup(const struct option *a);\n-struct option *parse_options_concat(struct option *a, struct option *b);\n+struct option *parse_options_concat(const struct option *a, const struct option *b);\n\n /*----- some often used options -----*/\n int parse_opt_abbrev_cb(const struct option *, const char *, int);\n--\n2.25.0\n"},{"id":"391403","messageId":"162fb36a-6dd1-c178-9032-d3f4213930a6@web.de","threadId":"52768","inReplyTo":"11b82734-f61c-5e73-2d0c-22208c06d495@web.de","subject":"[PATCH 4/4] parse-options: simplify parse_options_dup()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T15:58:42Z","receivedAt":"2020-02-09T15:58:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Simplify parse_options_dup() by making it a trivial wrapper of\nparse_options_concat() by making use of the facts that the latter\nduplicates its input as well and that appending an empty set is a no-op.\n\nSigned-off-by: René Scharfe \n---\n parse-options-cb.c | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 7d56681130..a28b55be48 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -170,15 +170,9 @@ static size_t parse_options_count(const struct option *opt)\n\n struct option *parse_options_dup(const struct option *o)\n {\n-\tconst struct option *orig = o;\n-\tstruct option *opts;\n-\tsize_t nr = parse_options_count(o);\n-\n-\tALLOC_ARRAY(opts, nr + 1);\n-\tCOPY_ARRAY(opts, orig, nr);\n-\tmemset(opts + nr, 0, sizeof(*opts));\n-\topts[nr].type = OPTION_END;\n-\treturn opts;\n+\tstruct option no_options[] = { OPT_END() };\n+\n+\treturn parse_options_concat(o, no_options);\n }\n\n struct option *parse_options_concat(const struct option *a,\n--\n2.25.0\n"},{"id":"391411","messageId":"CAPig+cQpzc6eZyOo9N=4sR3pBFza299rRn_wP0w2W7Zf5CWThg@mail.gmail.com","threadId":"52768","inReplyTo":"c729aaab-68d7-9cfa-8981-97eaa72a5ebe@web.de","subject":"Re: [PATCH 2/4] parse-options: factor out parse_options_count()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-09T17:56:00Z","receivedAt":"2020-02-09T17:56:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 9, 2020 at 10:56 AM René Scharfe wrote:\n> Add a helper function to count the number of options (excluding the\n> final OPT_END()) and use it to simplify parse_options_dup() and\n> parse_options_concat().\n>\n> Signed-off-by: René Scharfe \n> ---\n> diff --git a/parse-options-cb.c b/parse-options-cb.c\n> @@ -159,16 +159,20 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n> struct option *parse_options_dup(const struct option *o)\n> {\n> const struct option *orig = o;\n> struct option *opts;\n> - int nr = 0;\n> -\n> - while (o && o->type != OPTION_END) {\n> - nr++;\n> - o++;\n> - }\n> + size_t nr = parse_options_count(o);\n>\n> ALLOC_ARRAY(opts, nr + 1);\n> COPY_ARRAY(opts, orig, nr);\n\nThis could use a little more cleanup. After this change, 'o' is never\nagain consulted or changed, and 'orig' points at the original value of\n'o', which means 'o' and 'orig' have the same value now always.\nTherefore, the additional cleanup would be to drop the declaration and\nassignment of 'orig' and reference 'o' in COPY_ARRAY() rather than\n'orig'.\n"},{"id":"391413","messageId":"4577baea-0cc6-5fd9-a1c4-699b15dae380@web.de","threadId":"52768","inReplyTo":"CAPig+cQpzc6eZyOo9N=4sR3pBFza299rRn_wP0w2W7Zf5CWThg@mail.gmail.com","subject":"Re: [PATCH 2/4] parse-options: factor out parse_options_count()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T18:36:36Z","receivedAt":"2020-02-09T18:36:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.02.20 um 18:56 schrieb Eric Sunshine:\n> On Sun, Feb 9, 2020 at 10:56 AM René Scharfe wrote:\n>> Add a helper function to count the number of options (excluding the\n>> final OPT_END()) and use it to simplify parse_options_dup() and\n>> parse_options_concat().\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> diff --git a/parse-options-cb.c b/parse-options-cb.c\n>> @@ -159,16 +159,20 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n>> struct option *parse_options_dup(const struct option *o)\n>> {\n>> const struct option *orig = o;\n>> struct option *opts;\n>> - int nr = 0;\n>> -\n>> - while (o && o->type != OPTION_END) {\n>> - nr++;\n>> - o++;\n>> - }\n>> + size_t nr = parse_options_count(o);\n>>\n>> ALLOC_ARRAY(opts, nr + 1);\n>> COPY_ARRAY(opts, orig, nr);\n>\n> This could use a little more cleanup. After this change, 'o' is never\n> again consulted or changed, and 'orig' points at the original value of\n> 'o', which means 'o' and 'orig' have the same value now always.\n> Therefore, the additional cleanup would be to drop the declaration and\n> assignment of 'orig' and reference 'o' in COPY_ARRAY() rather than\n> 'orig'.\n\nTrue, but that would go beyond the purpose of this patch, which is to\nextract a count function. While it enables the cleanup you mentioned,\nthe latter should go into its own patch. The last one in the series\ntakes care of it.\n\nRené\n"},{"id":"391474","messageId":"xmqqh7zy1gee.fsf@gitster-ct.c.googlers.com","threadId":"52768","inReplyTo":"162fb36a-6dd1-c178-9032-d3f4213930a6@web.de","subject":"Re: [PATCH 4/4] parse-options: simplify parse_options_dup()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-10T17:49:45Z","receivedAt":"2020-02-10T17:49:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Simplify parse_options_dup() by making it a trivial wrapper of\n> parse_options_concat() by making use of the facts that the latter\n> duplicates its input as well and that appending an empty set is a no-op.\n> ...\n> +\tstruct option no_options[] = { OPT_END() };\n> +\n> +\treturn parse_options_concat(o, no_options);\n\nCan't say if this is tricky or cute, but I like it ;-)\n\nWill queue all four. Thanks.\n"},{"id":"391506","messageId":"20200210222518.GB623231@coredump.intra.peff.net","threadId":"52768","inReplyTo":"11b82734-f61c-5e73-2d0c-22208c06d495@web.de","subject":"Re: [PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-10T22:25:18Z","receivedAt":"2020-02-10T22:25:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 09, 2020 at 04:53:46PM +0100, René Scharfe wrote:\n\n> Reduce code duplication.\n> \n> René Scharfe (4):\n> parse-options: use COPY_ARRAY in parse_options_concat()\n> parse-options: factor out parse_options_count()\n> parse-options: const parse_options_concat() parameters\n> parse-options: simplify parse_options_dup()\n\nI read all 4 and they looked good to me.\n\n-Peff\n"}]}