Hello Ted,

Ted Unangst wrote on Sat, Jun 21, 2025 at 12:26:05AM -0400:

> How about this? There's a lot of one sentence man pages in libpthread,
> which I think are probably not very useful. It's a lackluster experience
> having to read four or five different man pages just to use a condition
> variable. Why isn't this all in one place?

I agree with the general direction.

There are no hard rules, it is always a matter of judgement, but
the general idea is that functions should be documented together
when they are so closely related or so similar that keeping them
together saves text and/or makes the text easier to follow, but
pages should be split when they become large - sometimes, a dozen
functions already warrants a split, sometimes (especially for
ill-designed interfaces) two or even three dozen functions in a
single page can still be OK.

Avoiding unnecessary churn is also a consideration, but the case
of excessive splitting you are showing here looks bad enough that
a bit of churn feels tolerable.

> This diff more or less mechanically combines the pages. I think a
> bit more narrative structure can be added, but at least you can get
> some idea of what these functions are for and how they work together
> in a single page now.

You have to be more careful with Copyright and license headers.

The page pthread_cond_init.3 is under a three-clause BSD license
with JOHN BIRRELL and THE REGENTS in the warranty disclaimer.

The page pthread_condattr_init.3, however, is under a two-clause
BSD license with THE COPYRIGHT HOLDER(S) in the warranty disclaimer.

It would arguably be legal to combine both files if you put something
like the following on top:

  Parts of this file are distributed under this Copyright and license:
    ...
  Other parts of this file are distributed under this Copyright and license:
    ...

However, i advise against that.  If files are under different licenses,
even if the licences are compatible and allow combining, it helps
clarity to not combine them unless there are very strong reasons,
and i do not consider combining manual pages that *can* be kept separate
a strong enough reason to risk even minor confusion about Copyright
and licensing.

Here is a list of the pages you are touching:

  page name         clauses  warranty discl       Copyright holder
  pthread_cond_init       3  BIRRELL+REGENTS      Brian Cully  1997
  pthread_condattr_init   2  COPYRIGHT HOLDER(S)  Jason Evans  2000
  pthread_cond_broadcast  3  BIRRELL+REGENTS      Brian Cully  1997
  pthread_cond_destroy    3  BIRRELL+REGENTS      Brian Cully  1997
  pthread_cond_signal     3  BIRRELL+REGENTS      Brian Cully  1997
  pthread_cond_timedwait  3  BIRRELL+REGENTS      Brian Cully  1997
  pthread_cond_wait       3  BIRRELL+REGENTS      Brian Cully  1997

So fixing this seems as easy as taking pthread_condattr_init.3
out of the merge.  But even then, you still have to check (carefully,
with diff(1)) that the Copyright, license, and warranty disclaimer
headers are word-for-word identical, and then merge Copyright holder
and Copyright year lists (in this case, they are all just Brian Cully
and 1997, so nothing to merge).

Finally, i'd suggest copying all the $FreeBSD$ CVS ID lines into
the combined file, ideally in the same order that text is moved in.
Doing that helps future maintainers in case anybody ever wants to
merge anything back or forth among operating systems, or in case
doubts about the provenance of the text should ever arise.

As it stands, your diff is NOT OK because it is mandatory to
have all functions documented in the SYNOPSIS listed in the NAME
section as well, usually in the same order.

It is also NOT OK because your order in the DESCRIPTION differs
from your order in the SYNOPSIS.  In very exceptional cases, that
may perhaps be acceptable, but i see no reason to make an
exception here.

Finally, your diff is NOT OK because it fails to touch the .Nd
one-line description.  With you diff,

  .Nd create a condition variable

remains but becomes badly misleading.  The .Nd line should ideally
be a concise summary of everything described in the manual page and
should also clearly distinguish the scope of the manual page from
the scopes of related manual pages, if that is possible without
turning wordy.

Maybe something like this could work:

  .Nd conditionally block and unblock threads

But maybe you have a better understanding of the purpose of this
stuff than i do and can come up with a summary that is even more
to the point.

> Starting with condition variables.

Please be careful to not touch any pages having David Leonard (d@)
listed as an author right now; i'm currently juggling large diffs
for those and would hate to experience conflicts.

My in-line remarks are suggestions, not conditions for going
ahead.


> Index: Makefile.inc
> ===================================================================
> RCS file: /home/cvs/src/lib/libpthread/man/Makefile.inc,v
> diff -u -p -r1.37 Makefile.inc
> --- Makefile.inc	12 Jan 2019 00:16:03 -0000	1.37
> +++ Makefile.inc	21 Jun 2025 04:19:48 -0000
> @@ -19,13 +19,7 @@ MAN+=	\
>  	pthread_barrierattr_getpshared.3 \
>  	pthread_cleanup_pop.3 \
>  	pthread_cleanup_push.3 \
> -	pthread_condattr_init.3 \

As explained, i'd prefer to keep the above separate.

> -	pthread_cond_broadcast.3 \
> -	pthread_cond_destroy.3 \
>  	pthread_cond_init.3 \
> -	pthread_cond_signal.3 \
> -	pthread_cond_timedwait.3 \
> -	pthread_cond_wait.3 \
>  	pthread_cancel.3 \
>  	pthread_create.3 \
>  	pthread_detach.3 \
> Index: pthread_cond_init.3
> ===================================================================
> RCS file: /home/cvs/src/lib/libpthread/man/pthread_cond_init.3,v
> diff -u -p -r1.13 pthread_cond_init.3
> --- pthread_cond_init.3	7 Jun 2025 00:16:52 -0000	1.13
> +++ pthread_cond_init.3	21 Jun 2025 04:17:28 -0000
> @@ -40,6 +40,24 @@
>  .In pthread.h
>  .Ft int
>  .Fn pthread_cond_init "pthread_cond_t *cond" "const pthread_condattr_t *attr"
> +.Ft int
> +.Fn pthread_cond_destroy "pthread_cond_t *cond"
> +.Ft int
> +.Fn pthread_cond_broadcast "pthread_cond_t *cond"
> +.Ft int
> +.Fn pthread_cond_signal "pthread_cond_t *cond"
> +.Ft int
> +.Fn pthread_cond_wait "pthread_cond_t *cond" "pthread_mutex_t *mutex"
> +.Ft int
> +.Fn pthread_cond_timedwait "pthread_cond_t *cond" "pthread_mutex_t *mutex" "const struct timespec *abstime"

Lines of 80 characters or more are very ugly, and there is no need for
them here.

My preferred solution would be to switch all the .Fn to .Fo in this
SYNOPSIS.  A close second would be to only switch those functions
from .Fn to .Fo that have more than one argument.

Consider the recommendations
in https://mandoc.bsd.lv/mdoc/style/synopsis_func.html :

  To show the prototype of a function, using Fo usually results in
  more readable source code than using Fn, in particular for functions
  having more than one argument.  Avoid input lines of 80 characters
  and more, and avoid input line continuation with a backslash
  character at the end of an input line.

  For functions having only one single argument, Fn is usually
  acceptable if the whole macro fits on one input line, but there
  is nothing wrong with using Fo for consistency with other, more
  complicated functions on the same page. If the type and name are
  both very short, using Fn with two arguments may result in source
  code that is easier to read.

(Yes, i need to update the recommendation regarding .Lb at the top
of that page.)

> +.Ft int
> +.Fn pthread_condattr_init "pthread_condattr_t *attr"
> +.Ft int
> +.Fn pthread_condattr_destroy "pthread_condattr_t *attr"
> +.Ft int
> +.Fn pthread_condattr_setclock "pthread_condattr_t *attr" "clockid_t clock_id"
> +.Ft int
> +.Fn pthread_condattr_getclock "pthread_condattr_t *attr" "clockid_t *clock_id"

Again, i'd prefer to not move those in here.

>  .Sh DESCRIPTION
>  The
>  .Fn pthread_cond_init
> @@ -50,12 +68,84 @@ If
>  is
>  .Dv NULL ,
>  the default attributes are used.
> +.Pp
> +The
> +.Fn pthread_cond_broadcast
> +function unblocks all threads waiting for the condition variable
> +.Fa cond .
> +The
> +.Fn pthread_cond_signal
> +function unblocks at least one thread waiting for the condition variable
> +.Fa cond .
> +.Pp
> +The
> +.Fn pthread_cond_wait
> +function atomically blocks the current thread waiting on the condition
> +variable specified by
> +.Fa cond ,
> +and unblocks the mutex specified by
> +.Fa mutex .
> +The waiting thread unblocks only after another thread calls
> +.Fn pthread_cond_signal
> +or
> +.Fn pthread_cond_broadcast
> +with the same condition variable.
> +The
> +.Fn pthread_cond_timedwait
> +function does the same, but returns after the system time reaches
> +.Fa abstime .
> +The current thread then reacquires the lock on
> +.Fa mutex .

Here, i suggest saying

  For both functions, the current thread then reacquires the lock on
  .Fa mutex .

because with your current text, readers might be mislead to think
that the final sentence only applies to pthread_cond_timedwait()
and not to pthread_cond_wait().

> +.Pp
> +The
> +.Fn pthread_cond_destroy
> +function frees the resources allocated by the condition variable
> +.Fa cond .

This needs to be moved up.
Alternatively, it could be moved down in the SYNOPSIS.

I think i slightly prefer documenting init() and destroy() together at
the top over constructing a bracket in the order "init, use, destroy".

> +.Pp
> +Condition variable attributes are used to specify parameters to
> +.Fn pthread_cond_init .
> +One attribute object can be used in multiple calls to
> +.Fn pthread_cond_init ,
> +with or without modifications between calls.
> +.Pp
> +The
> +.Fn pthread_condattr_init
> +function initializes
> +.Fa attr
> +with all the default condition variable attributes.
> +.Pp
> +The
> +.Fn pthread_condattr_destroy
> +function destroys
> +.Fa attr .
> +.Pp
> +The
> +.Fn pthread_condattr_setclock
> +function sets the clock attribute of
> +.Fa attr
> +to the value of the
> +.Fa clock_id
> +parameter.
> +The
> +.Fn pthread_condattr_getclock
> +function copies the value of the clock attribute from
> +.Fa attr
> +to the location pointed to by the
> +.Fa clock_id
> +parameter.
> +The clock attribute is the ID of the clock against which the timeout of
> +.Fn pthread_cond_timedwait
> +is compared;
> +the default value of the clock attribute is
> +.Dv CLOCK_REALTIME .

Again, i'd prefer to not move those here.

>  .Sh RETURN VALUES
>  If successful, the
>  .Fn pthread_cond_init
>  function will return zero and put the new condition variable ID into
>  .Fa cond ,
>  otherwise an error number will be returned to indicate the error.
> +Other functions similarly return zero for success and error numbers
> +for failure.

I suggest shortening this to:

  These functions return 0 if successful or a positive error number 
  to indicate failure.

What is put into output parameters does not belong into the RETURN VALUES
section but into the DESCRIPTION, like so:

  The
  .Fn pthread_cond_init
  function creates a new condition variable in
  .Pf * Fa cond ,
  with attributes ...

>  .Sh ERRORS
>  .Fn pthread_cond_init
>  will fail if:
> @@ -71,13 +161,23 @@ variable.
>  The system temporarily lacks the resources to create another condition
>  variable.
>  .El
> +.Pp
> +Other functions may fail if:

I'd prefer "The other functions fail if:".  Without the definite
article, it is unclear whether this is maybe intended to mean "some
other", and if so, which.

Besides, i don't think this needs "will" or "may".
I know very large numbers of our pages say "will fail",
but i see no good reason for that.  When touching something
anyway, it would seem nice to me to switch to "FOO fails if".

I dislike "may" even more than "will" in this context.
The term "may" is standardese for "implementations are free
to either report this as an error, or to ignore the situation
and succeed anyway".  We want to document what our implementation
actually does, not what it would be allowed to do instead, right?

> +.Bl -tag -width Er
> +.It Bq Er EINVAL
> +The value specified by
> +.Fa cond
> +or another argument is invalid.

I'd prefer being specific:

  The value specified by
  .Fa cond ,
  .Fa mutex ,
  or
  .Fa abstime
  is invalid.

> +.It Bq Er EBUSY
> +The variable
> +.Fa cond
> +is locked by another thread.

Can you make it clear that this only applies to pthread_cond_destroy()?
For example by prefixing it with:

  pthread_cond_destroy() also fails if:

> +.It Bq Er ETIMEDOUT
> +The system time has reached or exceeded the time specified in
> +.Fa abstime .
> +.El

Can you make it clear that this only applies to pthread_cond_timedwait()?
For example by prefixing it with:

  pthread_cond_timedwait() also fails if:

>  .Sh SEE ALSO
> -.Xr pthread_cond_broadcast 3 ,
> -.Xr pthread_cond_destroy 3 ,
> -.Xr pthread_cond_signal 3 ,
> -.Xr pthread_cond_timedwait 3 ,
> -.Xr pthread_cond_wait 3
> +.Xr pthread_mutex_init 3

Probably .Xr pthread_condattr_init 3 should be added here, right?

>  .Sh STANDARDS
> -.Fn pthread_cond_init
> -conforms to
> +These functions conform to
>  .St -p1003.1-96 .

While here, would you mind also checking whether this can be updated
to -p1003.1-2024 ?

Thanks for doing this work,
  Ingo