Re: [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2016, 16:30 UTC
Message-ID: <xmqq4m4ic0gw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqzimbc0gw.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 7 quoted lines

> This certainly is good, but I wonder if a new variant of OPT_STRING
> and OPTION_STRING that does the strdup for you, something along the
> lines of ...
> ... may make it even more pleasant to use?  Only for two fields in
> this patch that may probably be an overkill, but we may eventually 
> benefit from such an approach when we audit and plug leaks in
> parse-options users.  I dunno.

After sleeping on it, I do think this is an overkill. The pattern I would expect for the most normal (boring) code to use is rather:

    struct app_specific app;
    const char *opt_x = NULL;
    struct option options[] = {
	...
	OPT_STRING(0, "xopt", &opt_x, N_("x option"), ...),
        ...
	OPT_END()
    };

    parse_options(ac, av, prefix, options, ...);
    app.x_field = xstrdup_or_null(opt_x);
    ... other values set to app's field based on
    ... not just command line options but from
    ... other sources.

The only reason why the OPT_STRDUP appeared convenient was because options[] element happened to use a field in the structure directly. The patch under discussion does an equivalent of

    app.x_field = xstrdup_or_null(opt_x);

but the "opt_x" happens to be the same "app.x_field" in this case, so in that sense, it follows the normal and boring pattern.

The "struct app_specific" may not even exist in the same scope as the caller of parse_options(), but may have to be initialized in a function that is three-level deep in the callchain, with opt_x variable passed through as a parameter. So OPT_STRDUP may not be a bad or horrible idea, but it is not such a great one, either.