Re: bsearch utility

[Paul Eggert]

Sorav Bansal <address@hidden> writes:

> The code is now upward-compatible with BSD look(1) --- except that BSD
> look(1) was locale-agnostic, while our utility is sensitive to the
> locale specified by environment.

That's fine, and I'd say expected.

> I have factored out the common code into 'compare.h'. I am sending you
> 3 files: compare.h, look.c and sort.c. Common portions from sort.c
> have been removed and put into compare.h

Hmm, I see that compare.h contains some static functions.  This is a
small point, but I'd rather treat those functions as potential
candidates for a library, which sort and look could share.  So it'd be
better to have a compare.c that's compiled separately, to contain
those functions.  Some of them would become extern.

More important, there is still a lot of code duplication, e.g.,
the 'compare' function, and the body of 'main'.  We can't remove
all the duplication, but still.....

> The problem with using binary_search on non-regular files is that I
> have no way of knowing the file size.

lseek with SEEK_END should tell you that, right?

> Interpolating search is a good idea, but it cannot work in the general
> case

True.  Let's leave it out for now, then.

> By the way, while testing I noticed a small issue (bug?) in sort.c
> -- If both -d and -i options are specified, only one of them is
> used. Since, -d is more restrictive than -i, I made a change in
> compare.h to reflect that. The change has been commented using
> 'XXX'.

I don't see why the change is necessary.  The old code looked like this:

          if (! key->ignore)
            key->ignore = nonprinting;

and the new like this:

          if (key->ignore != nondictionary)
            key->ignore = nonprinting;

Since there are only 3 possibilities for key->ignore (0, nonprinting,
and nondictionary), the old and new code are logically equivalent.
Might as well leave it alone: the old code was a bit faster, and no
more confusing than the new.

I don't see the need for the -B option, or the -l option.  Can't
we omit them?

Isn't fseeko kind of a loser, performance-wise?  Wouldn't it
be faster if you used lseek, and took block boundaries into
account?  I suppose this is more of a tuning thing tho.

I would remove all that POSIX pedantic stuff from the option
parsing.  It's not needed for 'look'.  Just use getopt_long
as most other apps do.

The coding style in look.c is quite a bit different from that used
in coreutils.  Please look to see how comments are done, indenting,
etc.

Thanks again for volunteering with this!