Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()

[Kevin Wolf]

Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >> 
> >> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> >> >> >> Kevin Wolf <kwolf@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.

I conclude that I'll never be able to submit code that passes your
review in the first attempt because I don't know the specific criteria
(and you don't seem to know them either before you see the patch).

Fine, I'll live with it. It's just one of the things that makes working
in your subsystems more frustrating than in others.

Kevin