[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: restructure patch for review [bug-grep]

From: Claudio Fontana
Subject: Re: restructure patch for review [bug-grep]
Date: Mon, 7 Mar 2005 12:31:30 +0100 (CET)


 --- Charles Levert <address@hidden> ha
> * 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
However, note that often what seems only reformatting
at first glance hides variable renaming for example:

if (!o.var)
instead of

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

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

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,

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

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


Nuovo Yahoo! Messenger: E' molto più divertente: Audibles, Avatar, Webcam, 
Giochi, Rubrica… Scaricalo ora! 

reply via email to

[Prev in Thread] Current Thread [Next in Thread]