[brussels-dev] Code review: CR 6782154 (parsing utilities consolidation)

[sowmini.varadhan at sun.com]

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