On Jan 14, 2008 12:09 PM, Stephen Hahn <[EMAIL PROTECTED]> wrote:
>
> I think this looks good. Some comments, questions, suggestions
> follow.
>
> - Stephen
>
> ----
>
> man.c:
>
> 225 - 227. MAN_USAGE lacks mention of "-p".
OK
>
> 420,
> 505,
> .... (Style.) Elide spaces on inside of parentheses:
> "( catmando == 0 )" -> "(catmando == 0)", etc. cstyle will warn
> about this.
OK
> 462. (Nit.) Update comment to include man's -p case (or delete)?
OK
> 504. (Style.) Elide space on inside of left outer parenthesis.
OK
> 538. (Sp.) Either "with out" -> "with no" or "with out" ->
> "without".
OK
> 1633. (Style.) Space after comma.
OK
> 2227. Why is this assignment needed?
Not sure. It had the effect of shortening the string being compared,
which was probably a bug waiting to happen. Removed
> 2244. (Sp.) "paranthesis" -> "parentheses".
OK
> 3061 - 3062. Maybe "dupnode" instead of "dev_ino cache"? (Only
> mentioned here.)
Yep - last bits of some renaming there. Fixed.
> 3064ff. Is there a reason not to just remove the duplicate sections
> from the list rather than setting their first character to NUL
> (and then having special handling in print_manpath())? (Real
> question: why an array (and its awkward realloc() requirements)
> and not a list?)
I was likely overly influenced by existing code. I agree a list would
be more appropriate and have updated dupnode code to use a list.
man_node always had an array that was produced by split().
print_manpath() processes a list of man_node structures.
Transitioning man_node to use a list rather than an array would mean
touching a lot of code that wasn't part of the scope of this effort.
The code to compact the man_node secv array would likely be messier
and more computationally expensive than the current approach.
Unless there is strong objection, I'll only change dupnode to use a
list and not change the rest.
>
> 3102,
> 3123,
> 3145. (Style.) Space after return keyword.
Was aiming to match up with existing style. Fixed in new and old
code.
> 3119 - 3124. How does this comment match up with the stat()? (The
> other question it raises is that of a directory tree that doesn't
> deliver an explicit bin, but has manual pages in the correct
> location: the if (stat()) test eliminates handling such a tree.)
>
> 3127. (Nit.) I thought 3120 was "first"? :)
Collapsed to one "first" comment. The intent of deriving MANPATH from
PATH is (mostly) to give the section 1* pages that are most likely to
correspond to the command that is executed. If bindir doesn't exist,
it doesn't seem to make sense to include the MANPATH at that point.
For those people that have a need to search man directories that don't
have corresponding bin directories, the following is recommended.
PATH=...
export PATH
unset MANPATH
MANPATH=`man -p`:/my/extra/man
export MANPATH
Would you find it better to allow people to trick man by setting PATH
to point to non-existent bin directories to pick up those man
directories?
That is:
if (stat(bindir, &sb) == 0) {
for (i = 0 ...
...
}
> 3150. (Sp.) "translatfor" -> "translation"?
Fixed
Thanks for the feedback. I will post a fresh webrev as soon as I
figure out why it just broke for me. :/
--
Mike Gerdts
http://mgerdts.blogspot.com/
_______________________________________________
opensolaris-code mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
