Re: [PATCH] sched_setattr.2: Document sched_util_{min,max}

[Brian Norris <briannorris@chromium.org>]

Hi Alejandro,

Thanks for the look! A few comments and questions.

On Sun, May 26, 2024 at 12:36:58PM +0200, Alejandro Colomar wrote:
> Hi Brian,
> 
> On Fri, May 24, 2024 at 12:08:28PM GMT, Brian Norris wrote:
> > --- a/man/man2/sched_setattr.2
> > +++ b/man/man2/sched_setattr.2
> >      u64 sched_runtime;
> >      u64 sched_deadline;
> >      u64 sched_period;
> > +
> 
> Please don't use blank lines in the source code.  They trigger a
> warning.

Oops, I probably should have gotten further into the documentation to
figure out how to run the linters. Indeed I see the warning now, and
I'll make sure I don't add more lint in the next version.

> > +These flags indicate that the
> > +.I
> > +sched_util_min
> > +or
> > +.I
> > +sched_util_max
> > +fields, respectively, are present, representing the expected minimum and
> > +maximum utilization of the thread.
> 
> Please use semantic newlines.
> 
> $ MANWIDTH=72 man man-pages | sed -n '/Use semantic newlines/,/^$/p'

I'll give that man page a better read for my next submission. Thanks for
the callout.

>    Use semantic newlines
>      In the source of a manual page, new sentences should be started on
>      new lines, long sentences should be split  into  lines  at  clause
>      breaks  (commas,  semicolons, colons, and so on), and long clauses
>      should be split at phrase boundaries.  This convention,  sometimes
>      known as "semantic newlines", makes it easier to see the effect of
>      patches, which often operate at the level of individual sentences,
>      clauses, or phrases.

I'll do my best to interpret what the best "phrase boundaries" are. I
don't think the writing always has enough punctuation breaks to nicely
break into 80-char pieces.

> > @@ -353,7 +398,6 @@ .SH ERRORS
> >  .I attr.sched_flags
> >  contains a flag other than
> >  .BR SCHED_FLAG_RESET_ON_FORK ;
> > -or
> 
> This change seems to be unrelated to this patch, right?

I suppose it's unrelated. At first I was going to add new EINVAL
descriptions to this paragraph, and I found that it had an odd
(incorrect?) use of too many "or". But then I simply broke out an
additional EINVAL section, which makes this change less related.

Side note: on second thought, it probably makes sense to split this
paragraph into multiple anyway, since the pattern

  "condition A; or condition B; or condition C [...]"

gets a bit hard to read with sufficient number of different conditions.

If it's preferred (and based on your comment, it probably is?), I'll
make corrections in separate patches.

> >  .I attr.sched_priority
> >  is invalid; or
> >  .I attr.sched_policy

Regards,
Brian