Timothy S. Nelson wrote:

On Tue, 2 Sep 2008, Michael Kerrisk wrote:

quoted

Hello Timothy,

Timothy S. Nelson wrote:

quoted

    Updated information obtained from:
a)    Reading the source of search.h on my Fedora system (uses __USE_GNU
    instead of _GNU_SOURCE) -- this works in my test program

This is wrong.  See <feature.h> and feature_test_macros(7).

    Great!  Thanks.  I'll update the patch so that it tells people to 
#include <feature.h>.

 From feature_test_macros(7):

    NOTES
        <features.h>  is  a  Linux/glibc-specific  header   file.
        Other  systems have an analogous file, but typically with
        a different name.   This  header  file  is  automatically
        included  by  other  header  files as required: it is not
        necessary to explicitly include it  in  order  to  employ
        feature test macros.

quoted

quoted

b)    Reading the documentation:
    
http://www.gnu.org/software/libc/manual/html_node/Hash-Search-Function.html#Hash-Search-Function 

Your mail contains no description of what changes your patch makes,
or why they should be useful.  (I am not a mind reader ;-).)

    I guess I assumed you'd take a skim over the patch to find that out, 
but I can see why you wouldn't :).

    Here's a summary of the changes:
-    _GNU_SOURCE didn't work for me, so I'm updating the documentation so
    that it will work for others (now #include <feature.h>)

If it didn't work for you then you probably didn't define the FTM before
including *any* header file.  Including <features.h> should never be
necessary.

-    It was constantly difficult to find the piece of information I wanted
    on this page, so I reordered a lot of the paragraphs; it's now both
    more readable, and easier to find the information I want.  In
    particular, this includes grouping information about particular
    functions together under a heading with the function name
-    Various small readability enhancements and snippets of information
    (ie. mentioning errno.h)

quoted

Also, you combine several logical changes in a single patch.
Please review http://www.kernel.org/doc/man-pages/patches.html
and also take a look at man-pages(7).

    Ok.  I guess I assumed that, since I was moving a lot of stuff 
anyway as part of the reorg, a few extra changes wouldn't show that 
much.  But I can see your point.  I'm reworking the patch, but until we 
agree (see below), there's no point me issuing another patch we disagree 
on.

quoted

quoted

    Having said that, though, it's mostly a reorganisation, and 
putting in little snippets of information that would've helped me.  I 
don't mind if the patch gets changed around or whatever, but I think 
the patch is an improvement on the current situation.

But you don't explain what you changed, or why you think it
improves things.

quoted

In particular, things I
wondered about are:
-    I have no idea about groff; if anyone can think of improvements, go
    for it (I did check it out with nroff -man though)
-    If it needs to be split into multiple pages (ie. 
hcreate*/hdestroy* on
    a separate page from hsearch*), that's fine by me too

It's easier for me if you send patches inline.

    Ok, I'll try to remember that.  Btw, sorry about the subject line 
too (hopefully better now).

quoted

quoted

--- man-pages-3.08/man3/hsearch.3     2008-08-27 16:09:02.000000000 
+1000
+++ man-pages-3.08-tsn1/man3/hsearch.3        2008-09-02 

14:11:07.000000000 

+1000

quoted

@@ -41,7 +41,7 @@
 .sp
 .B "void hdestroy(void);"
 .sp
-.B #define _GNU_SOURCE
+.B #define __USE_GNU

No.

    Fixed in the next version.

quoted

quoted

 .br
 .B #include <search.h>
 .sp

@@ -58,23 +58,62 @@
 .BR hsearch (),
 and
 .BR hdestroy ()
-allow the user to create a hash table (only one at a time)
+allow the user to create and manipulate a hash table (only one at a 

time)
 which associates a key with any data.
 .PP
+The three functions
+.BR hcreate_r (),
+.BR hsearch_r (),
+.BR hdestroy_r ()
+are reentrant versions that allow the use of more than one table.
+The last argument used identifies the table.
+.PP
+.SS "hcreate()/hcreate_r()"
+.PP

Why do you think this is better?  I think it is better to describe the
non-reentrant functions first, and then describe the differences for
the reentrant functions.

    Well, as someone who only wants to use the re-entrant versions, I 
must say, this made things much more difficult for me to get a grip on.  
As soon as I knew what the re-entrant versions were, I knew I didn't 
care about the other ones, so I didn't want to know about them.  Also, 
one of the first things I'm wondering when I come to this man page is 
"There's two sets of functions here; I need to choose the right one to 
start with; which is it?".  This answers that.

Okay.

quoted

quoted

 First the table must be created with the function
-.BR hcreate ().
+.BR hcreate ()
+/
+.BR hcreate_r ().
+.TP
+.B Argument "nel"

I don't really think such subdivisions of the text really help,
and they are not the norm in man-pages.

    If you're talking about the headings for the individual functions, 
then "man perlfunc" :).  But that's also why I was wondering whether 
there should be separate man pages.

Formatted, perlfunc(1) is 7000 lines long.  It is a different kind of beast.

    If you're talking about the headings for the different arguments, 
then I guess I realise it's not quite like the others (although 
gethostbyname does something similar for the members of the hostent 
struct; would you like it if I reformatted it like that?).

No.

I can name
two advantages:
-    It makes it easier to quickly locate information
-    It immediately highlighted the fact that the "ret" argument was not
    documented at all (which is something I've corrected in this patch)

Okay -- I already fixed that in my revision of the page.
(But I got the name wrong: s/retval/ret/)

quoted

Also, you've removed the
..SH RETURN VALUE
section, which really should be present in every .2 and .3 page
(though it is currently missing from several).

    I've divided the Return Value into two separate sections, as you've 
seen.  This is one of the reasons why I was wondering if there shouldn't 
be at least two separate man pages here; one for hsearch/hsearch_r, and 
one for everything else.

One could debate the point in either direction (and man-pages
has examples of doing things both ways).  In this case, I think
the information can be conveyed compactly enough that it makes sense
to describe everything on one page.

    If your objection is that it's not marked ".SS", that's probably 
because I know nothing about groff.  Visually (with nroff -man), it 
somewhat resembles the information on the printf page.

There is no point in citing the strange exceptions.  (Many things in
printf.3 could be tidied.)  I'm interested in consistency with
the general layout in man-pages, also described in man-pages(7).
I'm not inclined (without very good reason) to accept changes that
create more divergence from the "standard" layout.

quoted

quoted

 The argument \fInel\fP is an estimate of the maximum number of entries
 in the table.
-The function
-.BR hcreate ()
+The creation function
 may adjust this value upward to improve the
 performance of the resulting hash table.
-.PP
+.TP
+.B Argument "tab"
+As specified above, in the case of hcreate_r, this points to the
+table to be created.  The struct it points to must be malloc'd and

Why must it be *malloc'd*?  Surely allocation anywhere (stack, heap) 
will do?

    Sorry.  If you ever find out what I was thinking when I wrote this, 
let me know, 'cause I don't know what I was thinking :).

    To me, the ideal situation for the man pages around the hash table 
management would be at least 3 separate man pages:
-    hsearch/hsearch_r
-    hcreate/hcreate_r
-    An overview of the hashing functions (referring to the other two man
    pages)

I disagree.  I think that the info is short enough that everything can
be in one page.

    I'm not sure where hdestroy/hdestroy_r should go; probably in its 
own page, possibly with the overview, and possibly with hcreate & 
friends.  I'm also not pushing for this change, but if you like the idea 
better than the way I've done things at the moment, then I'm happy to 
format things that way too (although then I'd probably need a suggestion 
on what man section the overview page would go best in).

    In review, it seems to me like I agree with you, except for:
-    I think re-entrant functions should be discussed with their brethren,
    rather than being discussed afterwards

Okay -- I'm not averse to that change.  I'll revise the page in that
way and see how it looks.

-    I think there's some sort of sectioning needed, which you're not sure
    of

Oh -- I'm sure: I don't want that ;-).

Cheers,

Michael

    Thanks for your careful review of the patch.

    :)


---------------------------------------------------------------------
| Name: Tim Nelson                 | Because the Creator is,        |
| E-mail: wayland-r28gBBs99rhWo+R/V/U2/g@public.gmane.org    | I am                           |
---------------------------------------------------------------------

----BEGIN GEEK CODE BLOCK----
Version 3.12
GCS d+++ s+: a- C++$ U+++$ P+++$ L+++ E- W+ N+ w--- V- PE(+) Y+>++ 
PGP->+++ R(+) !tv b++ DI++++ D G+ e++>++++ h! y-
-----END GEEK CODE BLOCK-----

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html