Re: [PATCH v4 03/45] error: Document Error API usage rules

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 7/7/20 11:05 AM, Markus Armbruster wrote:
>> This merely codifies existing practice, with one exception: the rule
>> advising against returning void, where existing practice is mixed.
>>
>> When the Error API was created, we adopted the (unwritten) rule to
>> return void when the function returns no useful value on success,
>> unlike GError, which recommends to return true on success and false on
>> error then.
>>
>
>> Make the rule advising against returning void official by putting it
>> in writing.  This will hopefully reduce confusion.
>>
>> Update the examples accordingly.
>>
>> The remainder of this series will update a substantial amount of code
>> to honor the rule.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> ---
>
>> @@ -95,14 +122,12 @@
>>    * Create a new error and pass it to the caller:
>>    *     error_setg(errp, "situation normal, all fouled up");
>>    *
>> - * Call a function and receive an error from it:
>> - *     Error *err = NULL;
>> - *     foo(arg, &err);
>> - *     if (err) {
>> + * Call a function, receive an error from it, and pass it to caller
>
> maybe s/to caller/to the caller/

Yes.

>> + * when the function returns a value that indicates failure, say false:
>> + *     if (!foo(arg, errp)) {
>>    *         handle the error...
>>    *     }
>> - *
>> - * Receive an error and pass it on to the caller:
>> + * when it doesn't, say a void function:
>
> Hmm. It looks like you have a single sentence "Call a function... when
> the function returns", but this line now makes it obvious that you
> have a single prefix: "Call a function, ...and pass it to the caller:"
> with two choices "when the function returns" and "when it doesn't".
> I'm not sure if there is a nicer way to typeset it, adding yet another
> ":" at the end of the line looks odd.  The idea behind the text is
> fine, I'm just trying to paint the bikeshed to see if there is a
> better presentation.
>
>>    *     Error *err = NULL;
>>    *     foo(arg, &err);
>>    *     if (err) {
>> @@ -120,6 +145,19 @@
>>    *     foo(arg, errp);
>>    * for readability.
>>    *
>> + * Receive an error, and handle it locally
>> + * when the function returns a value that indicates failure, say false:
>> + *     Error *err = NULL;
>> + *     if (!foo(arg, &err)) {
>> + *         handle the error...
>> + *     }
>> + * when it doesn't, say a void function:
>
> It helps that you have repeated the same pattern as above.  But that
> means if you change the layout, both groupings should have the same
> layout.  Maybe:
>
> Intro for a task:
> - when the function returns...
> - when it doesn't
>
> Also, are there functions that have a return type other than void, but
> where the return value is not an indication of error?  If there are,

Yes, there are such functions.

> then the "say a void function" clause makes sense (but we should
> probably recommend against such functions); if there are not, then
> "say a void function" reads awkwardly.  Maybe:
>
> - when it does not, because it is a void function:

What about

  - when it does not, say because it is a void function:

>> + *     Error *err = NULL;
>> + *     foo(arg, &err);
>> + *     if (err) {
>> + *         handle the error...
>> + *     }
>> + *
>>    * Receive and accumulate multiple errors (first one wins):
>>    *     Error *err = NULL, *local_err = NULL;
>>    *     foo(arg, &err);
>>

Thanks!