[PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup()

[PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup()

[René Scharfe]

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

[PATCH 1/4] parse-options: use COPY_ARRAY in parse_options_concat()

[René Scharfe]

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 <l.s.r@web.de>
---
 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

[PATCH 2/4] parse-options: factor out parse_options_count()

[René Scharfe]

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 <l.s.r@web.de>
---
 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

Re: [PATCH 2/4] parse-options: factor out parse_options_count()

[Eric Sunshine]

On Sun, Feb 9, 2020 at 10:56 AM René Scharfe <l.s.r@web.de> 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 <l.s.r@web.de>
> ---
> 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'.

Re: [PATCH 2/4] parse-options: factor out parse_options_count()

[René Scharfe]

Am 09.02.20 um 18:56 schrieb Eric Sunshine:
> On Sun, Feb 9, 2020 at 10:56 AM René Scharfe <l.s.r@web.de> 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 <l.s.r@web.de>
>> ---
>> 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é

[PATCH 3/4] parse-options: const parse_options_concat() parameters

[René Scharfe]

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 <l.s.r@web.de>
---
 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

[PATCH 4/4] parse-options: simplify parse_options_dup()

[René Scharfe]

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 <l.s.r@web.de>
---
 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

Re: [PATCH 4/4] parse-options: simplify parse_options_dup()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> 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.

Re: [PATCH 0/4] parse-options: simplify parse_options_concat() and parse_options_dup()

[Jeff King]

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