threads / patch / 52768

patch, 4 partsparse-options: simplify parse_options_concat() and parse_options_dup()

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

## tl;dr

9 messages between Feb 9, 2020 and Feb 10, 2020. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

René Scharfe· Feb 9, 2020, 15:53 UTC · lore
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· Feb 9, 2020, 15:55 UTC · re: René Scharfe · lore

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

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(-)
Show changes to parse-options-cb.c +2 −6
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· Feb 9, 2020, 15:56 UTC · re: René Scharfe · lore

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

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(-)
Show changes to parse-options-cb.c +12 −13
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
Eric Sunshine· Feb 9, 2020, 17:56 UTC · re: René Scharfe · lore

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

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

René Scharfe· Feb 9, 2020, 18:36 UTC · re: Eric Sunshine · lore

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

Am 09.02.20 um 18:56 schrieb Eric Sunshine:
Show 30 quoted lines
> 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é
René Scharfe· Feb 9, 2020, 15:57 UTC · re: René Scharfe · lore

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

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(-)
Show changes to 2 files +3 −3

parse-options-cb.c, parse-options.h

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· Feb 9, 2020, 15:58 UTC · re: René Scharfe · lore

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

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(-)
Show changes to parse-options-cb.c +3 −10
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
Junio C Hamano· Feb 10, 2020, 17:49 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 7 quoted lines
> 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· Feb 10, 2020, 22:25 UTC · re: René Scharfe · lore

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

On Sun, Feb 09, 2020 at 04:53:46PM +0100, René Scharfe wrote:
Show 7 quoted lines
> 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

← back to recent threads