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
