Kevin Wolf <kw...@redhat.com> writes:
> Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> writes:
>>
>> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kw...@redhat.com> writes:
>> >>
>> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
>> >> >> Kevin Wolf <kw...@redhat.com> writes:
[...]
>> >> >> > diff --git a/monitor/qmp.c b/monitor/qmp.c
>> >> >> > index 8469970c69..922fdb5541 100644
>> >> >> > --- a/monitor/qmp.c
>> >> >> > +++ b/monitor/qmp.c
>> >> >> > @@ -135,16 +135,10 @@ static void monitor_qmp_respond(MonitorQMP
>> >> >> > *mon, QDict *rsp)
>> >> >> >
>> >> >> > static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>> >> >> > {
>> >> >> > - Monitor *old_mon;
>> >> >> > QDict *rsp;
>> >> >> > QDict *error;
>> >> >> >
>> >> >> > - old_mon = monitor_set_cur(&mon->common);
>> >> >> > - assert(old_mon == NULL);
>> >> >> > -
>> >> >> > - rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
>> >> >> > -
>> >> >> > - monitor_set_cur(NULL);
>> >> >> > + rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
>> >> >> > &mon->common);
>> >> >>
>> >> >> Long line. Happy to wrap it in my tree. A few more in PATCH 08-11.
>> >> >
>> >> > It's 79 characters. Should be fine even with your local deviation from
>> >> > the coding style to require less than that for comments?
>> >>
>> >> Let me rephrase my remark.
>> >>
>> >> For me,
>> >>
>> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
>> >> &mon->common);
>> >>
>> >> is significantly easier to read than
>> >>
>> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
>> >> &mon->common);
>> >
>> > I guess this is highly subjective. I find wrapped lines harder to read.
>> > For answering subjective questions like this, we generally use the
>> > coding style document.
>> >
>> > Anyway, I guess following an idiosyncratic coding style that is
>> > different from every other subsystem in QEMU is possible (if
>> > inconvenient) if I know what it is.
>>
>> The applicable coding style document is PEP 8.
>
> I'll happily apply PEP 8 to Python code, but this is C. I don't think
> PEP 8 applies very well to C code. (In fact, PEP 7 exists as a C style
> guide, but we're not writing C code for the Python project here...)
I got confused (too much Python code review), my apologies.
>> > My problem is more that I don't know what the exact rules are. Can they
>> > only be figured out experimentally by submitting patches and seeing
>> > whether you like them or not?
>>
>> PEP 8:
>>
>> A style guide is about consistency. Consistency with this style
>> guide is important. Consistency within a project is more important.
>> Consistency within one module or function is the most important.
>>
>> In other words, you should make a reasonable effort to blend in.
>
> The project style guide for C is defined in CODING_STYLE.rst. Missing
> consistency with it is what I'm complaining about.
>
> I also agree that consistency within one module or function is most
> important, which is why I allow you to reformat my code. But I don't
> think it means that local coding style rules shouldn't be documented,
> especially if you can't just look at the code and see immediately how
> it's supposed to be.
>
>> >> Would you mind me wrapping this line in my tree?
>> >
>> > I have no say in this subsystem and I take it that you want all code to
>> > look as if you had written it yourself, so do as you wish.
>>
>> I'm refusing the bait.
>>
>> > But I understand that I'll have to respin anyway, so if you could
>> > explain what you're after, I might be able to apply the rules for the
>> > next version of the series.
>>
>> First, PEP 8 again:
>>
>> Limit all lines to a maximum of 79 characters.
>>
>> For flowing long blocks of text with fewer structural restrictions
>> (docstrings or comments), the line length should be limited to 72
>> characters.
>
> Ok, that's finally clear limits at least.
>
> Any other rules from PEP 8 that you want to see applied to C code?
PEP 8 does not apply to C.
> Would you mind documenting this somewhere?
>
>> Second, an argument we two had on this list, during review of a prior
>> version of this patch series, talking about C:
>>
>> Legibility. Humans tend to have trouble following long lines with
>> their eyes (I sure do). Typographic manuals suggest to limit
>> columns to roughly 60 characters for exactly that reason[*].
>>
>> Code is special. It's typically indented, and long identifiers push
>> it further to the right, function arguments in particular. We
>> compromised at 80 columns.
>>
>> [...]
>>
>> [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style
>>
>> The width of the line not counting indentation matters for legibility.
>>
>> The line I flagged as long is 75 characters wide not counting
>> indentation. That's needlessly hard to read for me.
>>
>> PEP 8's line length limit is a *limit*, not a sacred right to push right
>> to the limit.
>>
>> Since I get to read this code a lot, I've taken care to avoid illegibly
>> wide lines, and I've guided contributors to blend in.
>
> As I said, I don't mind the exact number much. I do mind predictability,
> though. (And ideally also consistency across the project because
> otherwise I need to change my editor settings for individual files.)
>
> So if you don't like 79 columns, give me any other number. But
> please, do give me something specific I can work with. "illegibly wide"
> is not something I can work with because it's highly subjective.
Taste is subjective.
We can always make CODING_STYLE.rst more detailed. I view that as a
last resort when we waste too much time arguing.
Back to line length.
CODING_STYLE.rst sets a *limit*.
Going over the limit violates CODING_STYLE.rst. There are (rare) cases
where that is justified.
CODING_STYLE.rst neither demands nor prohibits breaking lines before the
limit is reached.
Until CODING_STYLE.rst prohibits breaking lines unless they exceed the
limit, I will continue to ask for breaking lines when that makes the
code easier to read and more consistent with the code around it, for
code I maintain, and admittedly in my opinion.
These requests appear to irk you a great deal. I don't understand, but
I'm sorry about it all the same. By arguing about it repeatedly, you've
irked some back. Brought it on myself, I guess. However, if that's
what it takes to keep the code I maintain legible and consistent, I'll
pay the price.
