[illumos-Developer] webrev: reimplementation of od

[Garrett D'Amore]

On Wed, 2010-10-20 at 01:20 +0200, Roland Mainz wrote:
> On Tue, Oct 19, 2010 at 6: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
> [snip]
> > So I'll wait for further feedback, but unless someone can give me a
> > concrete example of where my implementation is deficient or buggy
> > (provide information, please -- "its broken" or "it doesn't conform to
> > specs" are useless comments without supporting information!), I'll be
> > moving forward with integration tomorrow.
> 
> Some random review comments from staring at it for a minute:
> 1. Is it possible for |lcm| or |blocksize| to become larger than
> |INT_MAX| for a 64 "od" version ? Instictively I would use |size_t|
> for such stuff (but I may be wrong).

Well, int is more than adequate, since with 12 byte types the biggest
value lcm can take is 24.  (I suppose if we had 16 byte types
intermixed, it could grow to 48, but that is still far short of INT_MAX.

That said, using size_t (to avoid sign errors) isn't a bad idea, so I'll
go ahead and make the change.

> 2. Is a signed datatype really wanted for both variables ?

See above, I'll use size_t.

> 3. Not all messages are localised and more descriptive text may be nice.

Which ones aren't?  I don't localize something like "fread: %s", because
the problem being reported is against a specific C function, which
should not be locale sensitive.  (The %s will be filled out by
strerror(), which should be localized, however.)


> 4. You use |fgetc()| to skip characters in a loop... please use
> |flockfile()|, then |getchar_unlocked()| in the loop, followed by a
> final |funlockfile()| after the loop.This should save lots of
> expensive calls for each characters skipped.

This is a good suggestion.  I've made the change.

I'll post an updated webrev soon, along with other feedback I received.

	- Garrett
> 
> I do some more detailed testing this night...
> 
> ----
> 
> Bye,
> Roland
>