[illumos-Developer] webrev: reimplementation of od

[Garrett D'Amore]

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