Timothy S. Nelson wrote:
On Tue, 2 Sep 2008, Michael Kerrisk wrote:
Hello Timothy,
Timothy S. Nelson wrote:
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.
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)
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.
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.
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).
--- 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
@@ -41,7 +41,7 @@
.sp
.B "void hdestroy(void);"
.sp
-.B #define _GNU_SOURCE
+.B #define __USE_GNU
No.
Fixed in the next version.
.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.
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/)
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.
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@xxxxxxxxxxxxx | 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
