Subject: Re: search.c clean-up
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
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, and the other
points I mentioned.
 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.
* src/search.c (WRAP_PATTERN): New macro to factor out ...
(Ecompile): ...and here.