[illumos-Developer] webrev: reimplementation of od

[Garrett D'Amore]

New webrev, with fixes for all the code review comments (well at least
the useful actionable ones) so far:

	http://mexico.purplecow.org/gdamore/webrev/od

Would appreciate a timely review, especially by those parties that had
provided feedback earlier.

This one is a "real" webrev in a tree.

	- Garrett

On Tue, 2010-10-19 at 21:10 -0700, Garrett D'Amore wrote:
> On Tue, 2010-10-19 at 22:23 -0400, Gordon Ross wrote:
> > On Tue, Oct 19, 2010 at 12:00 AM, Garrett D'Amore <garrett at damore.org> wrote:
> > >
> > > In order to answer some feedback questions, and address some bugs that
> > > were pointed out in review, I've changed the implementation somewhat.
> > >
> > > The new webrev is here: http://pastebin.ca/1966514
> > >
> > > Thea diffs from the previous (yesterday's) version of the code are here:
> > > http://pastebin.ca/1966521
> > >
> > > The new logic should be simpler to read, since it just uses a circular
> > > FIFO instead of three separate buffers.  It uses power of two math for
> > > efficiency.
> > > [...]
> > 
> > Lines 67-71: I'd prefer to see comments on what these fields mean.
> > I figured it out, but it would make it easier to read. i.e.
> > 
> > /*
> >  * this structure describes our ring buffer...
> >  * always power-of-two length ... etc.
> >  */
> >         int     prod;        /* producer index */
> >         int     cons;        /* consumer index */
> >         int     mask;        /* to constrain indexes. */
> >         int     size;        /* XXX - is this used in the program? */
> > Or does the code always use mask+1 instead?
> > Suggest using size in place of mask+1, or just
> > get rid of size and use mask+1 in its place.
> 
> Overall this is a good suggestion, and I'll change it.  The "size" field
> was originally meant to be used, but it turns out I only ever use it in
> the calculation of mask.  I'll just remove it.
> 
> > 
> > Otherwise looks OK to me by inspection.
> > I have not tested it.
> 
> Thanks.  You weren't expected to test it. :-)
> 
> 	- Garrett
> > 
> > Gordon
> 
> 
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer