Re: [bug-grep] search.c clean-up

[Julian Foad]

Jim Meyering wrote:
> Here's a little change I wrote months ago.
> The only problem is that it relies on mempcpy which is not
> always available, so this will have to wait until gnulib
> support is brought up to date.

Thank you for offering this patch.  I hope you won't mind if I criticise it a 
bit.

While I admire the spirit of your change, the use of non-standard function 
mempcpy is a bit of a problem in portable code, and it seems to call strlen() 
on constant strings even more times than it did before, which is only a small 
and constant cost but not very tasteful.  Also, defining a macro within the 
body of a function, and then using it outside that function is a bit odd 
(though there is nothing syntactically wrong with it).  Also, your code 
calculates and allocates an extra byte, but doesn't use it.

The bigger benefit will be gained by factoring out almost the entire content of 
the two functions Ecompile and Gcompile.  If you would like to do this partial 
factorisation first, then would you consider re-writing it without non-standard 
library functions, and minimising use of string functions[1], and the other 
points I mentioned.

Thanks.


[1]  Instead of strlen, for the size of the macro arguments that are constant 
strings you could use "sizeof(Prefix) - 1", but that would silently go wrong if 
the macro were involed with a string pointer.  So perhaps "const char *beg[] = 
Prefix; ...sizeof(beg)-1..." which would at least catch and disallow variable 
strings.  Note that some compiling environments might optimise away strlen 
calls on constant strings, but only if they know at compile time that they are 
going to be linked to a "strlen" that behaves like the standard version of that 
function.  That's a dangerous assumption in some environments.  The use of 
intermediate variables can much more easily be optimised away.

- Julian

>         * src/search.c (WRAP_PATTERN): New macro to factor out duplication...
>         (Gcompile): ...here,
>         (Ecompile): ...and here.