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
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.







Privacy