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 > > * Throughout: there are a number of places where "?" is currently > returned, which basically indicates a failure (e.g., 489). > Shouldn't these cases now return some failure to the output engine, > which then elects to display this information however it sees fit? done.. using OFMT_UNKNOWN > * 47: There should only be a "libfoo.h" when there is a corresponding > libfoo.so. This header file should be named something more > appropriate like "ofmt.h". Likewise, fix comment on line 77 done. > * 90: Why isn't `ofmt' static? Also, existing convention in this > file would have named this `oh' or `ofmth', not `ofmt'. done. > * 90-102: All of these declarations and definitions belong properly > sorted into the list at 183-194. Not sure what your definition of "proper sorting" is (alphanumeric? by cache-line? but I've tried putting them logically together :-) > * 209, 213: These two declarations should be neighboring, and most > logically after the `ofields' declaration. Also, `ofmterr' would > better match the conventions in this file. ok. > * 302: Would prefer this to be declared at the top. More importantly, > there should be an actual #define for the size, provided by ofmt.h. done. > * 312: Stray space after "memory", but moreover, I would prefer to use > the standard libc strerror() logic here. done. > * 315: Unclear why OFMT_NOMEM is needed; isn't that what a NULL return > from ofmt_open() means? As discussed.. no more OFMT_NOMEM. the library will return null for nomem. > * 301-327: More generally, this code is just ugly. Suggest: suggestion taken > * 605: What is the point of `ret' here? removed. > * 753: This is a partial failure. I think we should probably return > OFMT_SUCCESS here so that at least some of the information can be > retrieved (the ordering of logic in this function is careful fill > in all of the fields that do not require IPMP group info first. done > * 1128: Comment is stale (references to `ih'), no longer properly > formatted, and ends in a semicolon. fixed. btw, ih is used, but the comment did not parse. that's been fixed. > * 1295: Formatting here is now odd. Suggest adding a tabstop after > "0," on all the lines so that everything aligns again. done. thanks in advance for reviewing. --Sowmini
