Paolo Bonzini <pbonz...@redhat.com> writes:
> On 01/03/21 14:26, Markus Armbruster wrote:
>>> Didn't really forget; being pretty sure that there's no usage in the
>>> wild and having good reasons to remove the code, giving a firm removal
>>> date should encourage people to speak up sooner rather than later.
>> Second thoughts after agreeing to change something are okay. Keeping
>> them for yourself not so much, because it deprives your reviewers of a
>> chance to raise further points.
>
> Sorry about that.
>
>> In this case, the point I didn't make because I wanted to reach
>> agreement on contents before nitpicking form: you're not using
>> warn_report() the way it wants to be used:
>>
>> /*
>> * Print a warning message to current monitor if we have one, else to
>> stderr.
>> * Format arguments like sprintf(). The resulting message should be a
>> ---> * single phrase, with no newline or trailing punctuation.
>> * Prepend the current location and append a newline.
>> */
>> void warn_report(const char *fmt, ...)
>
> I knew about the rules for no newline or trailing punctuation, but I
> didn't remember the other. I can certainly respin, that said:
>
> - the comment should say "sentence", not "phrase". For example "a
> single phrase" is a single phrase, while "the resulting message should
> be a single phrase" is a single sentence.
I avoided "sentence", because good error messages aren't always
grammatically complete sentences. My use of "phrase" may well be wrong.
I tried! Patches welcome :)
Dicking around on the web, I just found
https://www.postgres-xl.org/documentation/error-style-guide.html
Drop the Postgres-specific parts, and what's left is pretty close to my
thoughts on error message style.
> - I'm not sure how to interpret the rule above. First of all, the
> sentence mixes part that are mandatory part ("no newline", checked by
> checkpatch.pl) is mixed with the style guide ("no trailing punctuation"
> and "a single sentence"). Second, whether a single sentence is better
> often depends on the case. For example, comparing these four:
>
> WARNING: -writeconfig foo: -writeconfig is deprecated. It will go away
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated and will go away
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated; it will go away
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated
> WARNING: -writeconfig foo: it will go away in QEMU 6.2 with no replacement
>
> The first one is what was in this patch.
>
> The second does sound fine to me and it's probably what I'll use in v2,
> with or without the "in QEMU 6.2" part. However some could consider it
> to be worse style due to the longer sentence.
>
> The third one is playing with the rules; a semicolon would be separating
> two sentences. However usage of the semicolon is quite common in error
> messages, so maybe it would be good too?
Semicolons can be okay, as long the resulting message is still short.
Still short:
warning: -writeconfig foo: -writeconfig is deprecated without replacement
warning: -writeconfig foo: option is deprecated; there is no replacement
No longer short:
warning: -writeconfig foo: -writeconfig is deprecated; it will go away in
QEMU 6.2 with no replacement
There is no need to squeeze all the information into the "primary" error
message! That one should state what's wrong *concisely*. If you feel
you should explain further, or would like to advise on what could be
done to fix the problem, a separate "hint" message often reads better
than overloading the primary message with information.
When reporting to the user, use error_report() / warn_report() for the
"primary", and error_printf() for the "hint".
When setting an Error, use error_setg() for the "primary", and
error_append_hint() for the "hint". error_report_err() will then use
error_report() and error_printf() correctly.
> The last one also complies, but it is not clear what "it" refers to so
> it seems to be the worst in this case. In other cases it's the obvious
> choice, and we even have an API for it (error_append_hint... however it
> doesn't play well with error_fatal which I'm otherwise a big fan of).
> In this case, not so much.
Use of error_append_hint() requires ERRP_GUARD(). Without ERRP_GUARD(),
the hint indeed gets lost when errp == &error_fatal.
Properly guarded, we could have something like
warning: -writeconfig foo: option -writeconfig is deprecated
This option will go away with no replacement.
I'm glad you like &error_fatal, too! I have had to defend it a few
times :)
