Re: restructure patch for review [bug-grep]

[Charles Levert]

* On Monday 2005-03-07 at 00:54:25 +0100, Claudio Fontana wrote:
> 
> Built using diff -Naur, so apply using -p1

Please consider adding -p to diff options for your next
iteration.  It makes it easier for a human being to just
read the patch (as opposed to feeding it to the patch
program that just ignores the extra info).  Just reading
it is the basis for my initial comments.

Now that you have an account on Savannah, you should
probably move your patch from SourceForge to there, so
that we can all discuss it in one place and so that it is
found where people would first look for patches to grep.

There are a few changes in there that are purely cosmetic,
such as replacing indentation tabs for spaces without even
changing the indentation level, or changing the spacing in
conditional expressions.  Please reduce the size of your
main patch by isolating those changes in a separate patch.
Your main patch should rely on the principle of least
astonishment.


> * prline madness has been addressed, with one small
> function and one auxiliary function called only for
> partial line printing, named prline_part().

My patch #3644 on Savannah already addressed this in a
way that avoids code duplication when more colorizing
is performed and --byte-offset is made to combine with
--only-matching.  I already split the cleaning up in
several preliminary patches (patch #3767, patch #3770,
and patch #3771).  I don't believe that prline_part()
is the right half of prline() to factor out as it is not
the half that will eventually be used more than once,
but rather print_linehead().  (See an earlier discussion
thread entitled "--only-matching interacts badly with
context lines and line head" on this mailing list.)

There is unnecessary duplication of work here.  Please look
for and consider what has already been addressed before
getting to work on re-doing it.


> * search.c translation table for kwsinit() is now
> built once and for all before compiling any regex.
> This avoids the need to loop each time in kwsinit().

I would keep this local to "src/search.c" by having a
"static int icase_trans_initialized" there, probably within
kwsinit() itself.  No need for a new function.

This can easily go in its own patch as it is orthogonal
to any of the rest.


> * Gcompile and Ecompile functionality has been merged.
> They now call EGcompile_aux with an additional syntax
> parameter, where only the base syntax (RE_SYNTAX_GREP,
> RE_SYNTAX_POSIX_EGREP, RE_SYNTAX_AWK...) must be
> specified. A lot of duplication was thus removed.

In a private copy, I have done something similar, but
I have actually added an Acompile() front end function
to avoid using the "matcher" global variable at all in
"src/search.c".  This variable no longer needing to be
global (it can now be static to "src/grep.c"), the whole
"src/grepmat.c" then becomes unnecessary.  Essentially
doing the string compare more than once should be avoided.

Do any of the remaining "src/search.c" changes have any
dependencies on the rest of the patch?  If not, they should
as well be in their own patch, which should be very easy
to accomplish given that they already are in their own
file and have no common lines with the rest of the changes.


> I included the changes for RE_ICASE in
> lib/posix/regex.h also available in the redhat
> 2.5.1-oi patch, 
> 
> http://cvs.fedora.redhat.com/viewcvs/devel/grep/grep-2.5.1-oi.patch

Please keep this separate and make your patch apply on
top of it.  "lib/posix/regex.h" should just be updated
from glibc; also, for some reason, I've noticed that the
Red Hat patch does not insert the new lines in it in the
most logical place.

In my opinion, updating both "lib/posix/regex.h" and
"lib/regex.c" should be done separately and considered
high-priority since many patches such as this one have
come to depend on it.


> I would also
> advocate wrapping all those global variables in a
> struct in grep.c, namely struct grep_state s;

I have already done some work (as preliminary to
an eventual -p option, but still useful without it)
that I have declared to the mailing list but not yet
published that replaces all global or static variables in
"src/search.c" and "src/dfa.c" and puts them in objects
(structs) and makes the whole thing re-entrant.

I am waiting for the pipeline of pending patches to be
substantially drained before I publish them, but we should
avoid duplication of efforts here.

This includes variables such as match_icase, match_words,
match_lines, and eolbyte.



Stepan:  It would be nice to devise a summary roadmap
including an identification of things that should go in a
2.5.2 milestone release and of other things that should go
in a 2.6.0 milestone release (or any in between).  Maybe
bug fixes vs. new functionality, enhanced performance,
or heavy refactoring.