Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 12/17/2015 11:04 AM, Markus Armbruster wrote:
>
>>>> + * Create an error and add additional explanation:
>>>> + *     error_setg(&err, "invalid quark");
>>>> + *     error_append_hint(&err, "Valid quarks are up, down, strange, "
>>>> + *                       "charm, top, bottom\n");
>>>
>>> Do we want to allow/encourage a trailing dot in the hint?  I'm fine if
>>> we don't - but once we document its usage, we should probably then be
>>> consistent with what we document; qemu-option.c used a trailing dot,
>>> qdev-monitor.c did not.
>> 
>> I'll fix this example.
>> 
>>>> + *
>>>> + * Do *not* contract this to
>>>> + *     error_setg(&err, "invalid quark\n"
>>>> + * "Valid quarks are up, down, strange, charm, top, bottom");
>
> Of course, if you put a trailing dot above, you'd also want it here in
> the 'don't contract' section.

Yes.

>>>> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
>>>>   * If @errp is anything else, address@hidden must be NULL.
>>>>   * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>>>>   * human-readable error message is made from printf-style @fmt, ...
>>>> + * The resulting message should not contain newlines.
>>>
>>> Should we also discourage trailing punctuation?
>> 
>> Yes.  How to best phrase it?
>
> Maybe:
>
> The resulting message should be a single phrase, with no newline or
> trailing punctuation.

What about ending the message with an exclamation mark?

>>> Should we also mention no need for leading 'error: ' prefix?
>> 
>> Unlike the other anti-patterns, this one doesn't seem frequent in new
>> code.
>
> Fair enough.
>
>
>>>>  /**
>>>>   * Append a printf-style human-readable explanation to an existing error.
>>>> - * May be called multiple times, and safe if @errp is NULL.
>>>> + * @errp may be NULL, but neither &error_fatal nor &error_abort.
>>>
>>> As a native speaker, 'not' sounds better than 'neither' in that
>>> sentence.  But I think both choices are grammatically correct.
>> 
>> You mean "not &error_abort or &error_abort"?
>
> Yes, that works:
>
> @errp may be NULL, but not &error_fatal or &error_abort.