Re: restructure patch for review [bug-grep]

[Claudio Fontana]

Hello, 

 --- Charles Levert <address@hidden> ha
scritto: 
> * 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.

Ok, will do it from now on.

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

Ok, I'll post it (them) there asap.

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

Since I'm used to code alone, reformatting is an
automatic reflex, so mostly I did not really intend
it.
However, note that often what seems only reformatting
at first glance hides variable renaming for example:

if (!o.var)
instead of
if(!var)

A few purely estetic fixes remain though, I will
revert them in my next version.

> > * 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 read the patch and the related patches. I do not
address --byte-offset in my patch (its meaning when
coupled with --only-matching remains the same of
current CVS). 
As for your print_linehead solution, that has a lot of
sense to fix (byte-offset + only-matching), and it is
not incompatible. We are addressing two slightly
different things.

I would suggest to avoid the if (color ||
onlymatching) big block with the gotos, which looks a
bit confusing honestly, and replace it with a call to
a modified prline_part, which would in turn call
print_linehead.

> There is unnecessary duplication of work here. 

Maybe, but I see how we can avoid it, and also see the
ideas that can merge.

> Please look
> for and consider what has already been addressed
> before
> getting to work on re-doing it.

Sorry, I do not want to ignore other people's work.
I now have a better grasp of savannah bugs / patches
system, so in the future I'll try to interface with
any existant work.

On with other details:

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

I agree. I was basing on a former design (the
match_icase issue). Exposing that function is
expecially ugly. I'll revert that.

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

Ok. Since you seem to be dealing more with search.c,
dfa.c than I do, I should maybe not step into search.c
at all, and use the interface you provide. 

Please take from this version if there is something in
EGcompile_aux you might reuse. 

> Do any of the remaining "src/search.c" changes have
> any
> dependencies on the rest of the patch? 

Fortunately not. This is the reason I kept matcher,
match_icase, eolbyte.. out of the option
restructuring.

I'll separate changes to search.c and post them
separately so you can use them for your work on
search.c / dfa.c

> > I included the changes for RE_ICASE in
> > lib/posix/regex.h also available in the redhat
> > 2.5.1-oi patch, 

> Please keep this separate and make your patch apply
> on
> top of it.

Ok, but please note that the mentioned patch changes
to #include <regex.h> in search.c but not in dfa.c,
and

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

. It should be fixed. 

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

lib/ should be really updated. 
I've encountered trouble with modules 'error' (big
trouble) and 'savedir' too.

I will post the savedir fix I have made separately
too, until lib/ is updated.

Also, would it be possible to use the external
interfaces whenever possible, relying on lib internals
only when necessary? (posix api?) This kind of
philosophy would make lib versions issues less
damaging. 

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

That's ok, we are not duplicating. I will avoid
touching those.

We can set the confines of struct grep_state to grep.c
specific static variables like buffers, buffer file
descriptor, current filename.

It is needed because some functions of the main loop I
am dealing with rely on those statics, and would
profit from having it clear when a reference to such a
var is made. So indicatively it will be s.buffer,
s.desc, s.after_last_match and such.

To be able to set search options in grepopt.c though I
would need an interface to your search struct
(search.h) to set matcher, match_*, eolbyte.

I do not know about versioning issues, but as I
already told Stepan in another mail, I see a priority
in going for a heavy restructure to make grep more
maintenable, and is what I am offering my help for.

I think that a lot of bugs come out of the design
problems, and continuing with hacks that "make the
fix" or add new features raping the design actually
may make the problem worse.
I see you are addressing structural problems too, so I
hope you share this idea.

Claudio




                
___________________________________ 
Nuovo Yahoo! Messenger: E' molto più divertente: Audibles, Avatar, Webcam, 
Giochi, Rubrica… Scaricalo ora! 
http://it.messenger.yahoo.it