[v2,1/7] parse-options: allow parse_options_concat(NULL, options)
diff mbox series

Message ID 20181127165211.24763-2-pclouds@gmail.com
State New
Headers show
Series
  • Introduce new commands switch-branch and checkout-files
Related show

Commit Message

Duy Nguyen Nov. 27, 2018, 4:52 p.m. UTC
There is currently no caller that calls this function with "a" being
NULL. But it will be introduced shortly. It is used to construct the
option array from scratch, e.g.

   struct parse_options opts = NULL;
   opts = parse_options_concat(opts, opts_1);
   opts = parse_options_concat(opts, opts_2);

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options-cb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefan Beller Nov. 27, 2018, 7:43 p.m. UTC | #1
On Tue, Nov 27, 2018 at 8:53 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> There is currently no caller that calls this function with "a" being
> NULL. But it will be introduced shortly. It is used to construct the
> option array from scratch, e.g.
>
>    struct parse_options opts = NULL;
>    opts = parse_options_concat(opts, opts_1);
>    opts = parse_options_concat(opts, opts_2);

While this addresses the immediate needs, I'd prefer to think
about the API exposure of parse_options_concat,
(related: do we want to have docs in its header file?)
and I'd recommend to make it symmetrical, i.e.
allow the second argument to also be NULL?

In the example given here, you'd just short it to

    struct parse_options opts = opts_1;
    opts = parse_options_concat(opts, opts_2);

if not for this patch. Are opts_{1,2} ever be NULL?
Junio C Hamano Nov. 28, 2018, 4:47 a.m. UTC | #2
Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> There is currently no caller that calls this function with "a" being
> NULL. But it will be introduced shortly. It is used to construct the
> option array from scratch, e.g.
>
>    struct parse_options opts = NULL;

Missing asterisk somewhere?

>    opts = parse_options_concat(opts, opts_1);
>    opts = parse_options_concat(opts, opts_2);
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  parse-options-cb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 8c9edce52f..c609d52926 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -126,7 +126,7 @@ 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++)
> +	for (i = 0; a && a[i].type != OPTION_END; i++)
>  		a_len++;
>  	for (i = 0; b[i].type != OPTION_END; i++)
>  		b_len++;
Duy Nguyen Nov. 28, 2018, 3:22 p.m. UTC | #3
On Tue, Nov 27, 2018 at 8:44 PM Stefan Beller <sbeller@google.com> wrote:
>
> On Tue, Nov 27, 2018 at 8:53 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> >
> > There is currently no caller that calls this function with "a" being
> > NULL. But it will be introduced shortly. It is used to construct the
> > option array from scratch, e.g.
> >
> >    struct parse_options opts = NULL;
> >    opts = parse_options_concat(opts, opts_1);
> >    opts = parse_options_concat(opts, opts_2);
>
> While this addresses the immediate needs, I'd prefer to think
> about the API exposure of parse_options_concat,
> (related: do we want to have docs in its header file?)
> and I'd recommend to make it symmetrical, i.e.
> allow the second argument to also be NULL?

I'll just drop this patch. There's a better way to do the same without
adding special handling like this.

Patch
diff mbox series

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 8c9edce52f..c609d52926 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -126,7 +126,7 @@  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++)
+	for (i = 0; a && a[i].type != OPTION_END; i++)
 		a_len++;
 	for (i = 0; b[i].type != OPTION_END; i++)
 		b_len++;