[illumos-Developer] webrev: reimplementation of od

[Roland Mainz]

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).
2. Is a signed datatype really wanted for both variables ?
3. Not all messages are localised and more descriptive text may be nice.
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.

I do some more detailed testing this night...

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)