Re: [PULL 27/29] vl: deprecate -writeconfig

[Paolo Bonzini]

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'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?

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.

Paolo