Re: [PATCH 6/6] strbuf-safe: add init and release methods

[Junio C Hamano <gitster@pobox.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +int jw_release(struct json_writer *jw)
>  {
> -	strbuf_release(&jw->json);
> -	strbuf_release(&jw->open_stack);
> +	enum safe_result result = SUCCESS;
> +
> +	/* attempt both removals without short-circuiting. */
> +	result = sstrbuf_release(&jw->json) || result;
> +	result = sstrbuf_release(&jw->open_stack) || result;
> +
> +	return result;
>  }

This is puzzling in a few ways.

"enum safe_result" so far has been SUCCESS==0 and MEMORY_ERROR==1.
Presumably in some future we would gain other kind of error symbols,
but when that happens is this meant to act as an enumeration of
different kinds errors?  Or an enumeration of bitmasks that can
signal different kinds of errors?

If we mean "enum safe_result" is an enumeration of different kinds
of errors, then the "result" variable and the returned value from
here would be able to report a *single* kind of error, and it may
be common to report the first error we encounter, in which case

    enum safe_result result = SUCCESS;
    enum safe_result res;

    res = sstrbuf_release(&jw->json);
    if (!result && res)
	result = res;
    res = sstrbuf_release(&jw->open_stack);
    if (!result && res)
	result = res;
    return result;

would be slightly longer, far easier to reason about, and is a lot
more futureproof.  What you wrote, with "||", does not really allow
anything other than "is it still zero, or coalesce any non-zero
value to 1".

On the other hand, if we mean "enum safe_result" is an enumeration
of bitmasks, each bit representing different kind of error, then

    enum safe_result result = 0;

    result |= sstrbuf_release(&jw->json);
    result |= sstrbuf_release(&jw->open_stack);
    return result;

would probably be what you want.  That way you can add different
functions that returns different bit to signal a different kind of
error and or it in.

    result |= some_function();