Re: Hello fdclose

On Tue, 18 Mar 2014, John Baldwin wrote:

> On Monday, March 17, 2014 7:23:19 pm Mariusz Zaborski wrote:
> ...
> I think the code is fine.  I have a few suggestions on the manpage wording:
>
> .Sh RETURN VALUES
> -Upon successful completion 0 is returned.
> +The
> +.Fn fcloseall
> +function return no value.
> +.Pp
> +Upon successful completion
> +.Fn fclose
> +return 0.
> +Otherwise,
> +.Dv EOF
> +is returned and the global variable
> +.Va errno
> +is set to indicate the error.

The .Rv macro should be used whenever possible.  Unfortunately, it doesn't
support the EOF return, but only -1, so stdio man pages can rarely use it,
and this one is no exception.  Using it gives standard wording that is
quite different from the above:

standard wording:
      The close() function returns the value 0 if successful; otherwise the
      value -1 is returned and the global variable errno is set to indicate the
      error.
above wording (previous):
      Upon successful completion 0 is returned.                Otherwise,
      EOF is returned      and the global variable errno is set to indicate the
      error.
above wording (new):
      Upon successful completion fclose() return [sic] 0.      Otherwise,
      EOF is returned      and the global variable errno is set to indicate the
      error.

These are excessively formal in different ways:
- I don't like "the foo() function".  Why not just "foo()"?  The
   standard wording uses this, and so does the new wording, but the
   previous wording omits the function name (that only works for man pages
   that only have a single function, as they should).
- I don't like "the value N".  Why not just "N"?  The standard wording
   uses this, but the previous and new wordings don't.
- "returns N" is better than "N is returned".  Some man pages use worse
   wordings like "N will be returned".
- "the global variable errno" is excessively detailed/verbose, without
   the details even being correct.  Why not just "errno", with this
   identifier documented elsewhere?  errno isn't a global variable in
   most implementations.  It is can be, and usually is, a macro that
   expands to a modifiable lvalue of type int.  In FreeBSD, the macro
   expands to a function that returns a pointer to int.
- "Upon sucessful completion" is correct but verbose.  The standard
   wording doesn't even use it.
- the standard wording uses a conjunction instead of a new sentence
   before "otherwise" (this is better).  It is missing a comma after
   "otherwise" (this is worse).

> +.Pp
> +The
> +.Fn fdclose
> +function return the file descriptor if successfull.
> Otherwise,
> .Dv EOF

"successfull is consistently misspelled.

> One of English's arcane rules is that most verbs append an 's' when used with
> singular subjects, so "function returns" shoud be used instead of "function
> return", etc.  I do think for this section it would be good to combine the
> descriptions of fclose() and fdclose() when possible, so perhaps something
> like:
>
>  "The fcloseall() function returns no value.
>
>   Upon successful completion, fclose() returns 0 and fdclose() returns the
>   file descriptor of the underlying file.  Otherwise, EOF is returned and
>   the global variable errno is set to indicate the error.  In either case
>   no further access to the stream is possible."

OK.  You kept "return[s] N" and and deverbosified "the foo() function".
"Upon successful completion" is needed more with several functions.
"the global variable errno" remains consistently bad.

There should be a comma after "In either case".

> This allows "in either case" to still read correctly and makes it clear it
> applies to both fclose() and fdclose().

Better "In every case".

>
> .Sh ERRORS
> +.Bl -tag -width Er
> +.It Bq Er EOPNOTSUPP
> The
> +.Fa _close
> +method in
> +.Fa stream
> +argument to
> +.Fn fdclose ,
> +was not default.
> +.It Bq Er EBADF

The ERRORS section should be sorted.

> For the errors section, the first error list needs some sort of introductory
> text.  Also, this shouldn't claim that fdclose() can return an errno value for
> close(2).
>
> "ERRORS
>
>   The fdclose() function may will fail if:

I don't like the tense given by "will" in man pages.  POSIX says "shall
fail" in similar contexts, and "will fail" is a mistranslation of this
("shall" is a technical term that doesn't suggest future tense).

deshallify.sh does the not-incorrect translation s/shall fail/fails/
(I think this is too simple to always work).  It doesn't translate
anything to "will".

I can't parse "may will" :-).  deshallify.txt doesn't translate "may"
or "should" to anything (these are also technical terms in some
contexts, so they might need translation.  IIRC, "may" is optional
behaviour, mostly for the implementation, while "shall" is required
behaviour, only for the implementation, but "should" is recommended
practice, mostly for applications).  Man pages are very unlikely to
be as consistent as POSIX with these terms.

>   [EOPNOTSUPP]   The stream to close uses a non-default close method.
>
>   [EBADF]        The stream is not backed by a valid file descriptor.
>
>   The fclose() and fdclose() functions may also fail and set errno for any of
>   the errors specified for fflush(3).

Most stdio man pages just point to underlying functions for errors.  This
avoids duplication.  The above EBADF seems to be redundant, since fflush(3)
already has it, with a different and longer description.

>
>   The fclose() functino may also fail and set errno for any of the errors
>   specified for close(2)."

fclose() is now a small function :-).

> _at__at_ -84,7 +130,9 _at__at_
> .Sh NOTES
> The
> .Fn fclose
> -function
> +and
> +.Fn fdclose
> +functions
> does not handle NULL arguments; they will result in a segmentation
> violation.
> This is intentional - it makes it easier to make sure programs written
>
> "do not handle".

"they" is now ambiguous.

In the old version:

Em-dash seems to be handled poorly by mdoc.  It seems to be necessary
to hard code it.  It shouldn't be hard coded as a hyphen.

The double "it" is ambiguous.

Bruce