> On (02/09/09 14:14), Peter Memishian wrote:
> > For starters, here's my input from an initial run through ipmpstat.c.
> > Once we converge here, I'll go through the rest.
>
> I've updated the webrevs with the items you flag here (to the
> best of my understanding- many of them are stylistic preferences
> where I personally don't have any dogmatic stands).
>
> as a reminder, webrevs are at
> http://cr.opensolaris.org/~sowmini/parsegoop/webrev
> http://zhadum.east/export/ws/sowmini/brussels/parsegoop/webrev
Thank you for weathering my pickiness on this, it's looking good. Also, I
confess that `oh', while consistent, looks sillier than I'd thought it
would. I guess we should just use `ofmt' after all. Sorry for wasting
your time on that.
Remaining comments (mostly nits) on ipmpstat.c:
* 47: <ofmt.h> should be sorted alphabetically with respect to the
other #includes.
* 75: I think this comment needs to be expanded on to tie in with
the surrounding comments. Will think about and get back to you.
* 77: Comment here still mentions libprintutil.h.
* 154: Unclear why the comment was removed.
* 166: Since the appropriate size for this buffer is known to ofmt
and not ipmpstat, this #define should be provided by <ofmt.h>,
not by ipmpstat.
* 177, 180, 183, 186, 190: Please restore the original layout of
these fields as per lines 184-185 in the old version.
* 206: Why is `errbuf' global?
* 301: s/formattted/formatted/. Also, the use of "utilities"
seems a bit strange here; if you can't stomach "engine", maybe
"facility" or "logic"?
* 303: Seems like "(opt & IPMPSTAT_OPT_PARSABLE) != 0" was meant.
* 307-311: Not sure what was wrong with what I'd proposed earlier,
but if you're going to go with this, please capitalize the first
character, put two spaces after periods, use an 80-column
margin, s/some/An/, and s/parseable/parsable/.
* 314: Prefer "cannot create output handle: %s\n" to match
existing style (note that line 298 is different since it is
connecting two separate thoughts with "--", not returning an
error code).
* 318-319: Remove the braces and this will fit on one line.
* 673, elsewhere: Would prefer `ofmtarg' to `of_arg' to match
other variable prefixes like `ofmterr'.
* 1119-1122: Rejustify text with an 80-column and use two spaces
after the periods.
--
meem
