[Top][All Lists]

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

Re: sync dfaexec from gawk: fixes real bugs in grep

From: Paolo Bonzini
Subject: Re: sync dfaexec from gawk: fixes real bugs in grep
Date: Fri, 12 Mar 2010 15:55:24 +0100
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20100120 Fedora/3.0.1-1.fc12 Lightning/1.0b2pre Thunderbird/3.0.1

while resigned to it for now, I'm still against the fact
that this new API makes "*END" a non-const pointer, and would like to
reserve the right to fix that later.  Making the above change would
serve to entrench the new API.

Fair enough.

+           if ((char *) p > end)
+             break;

So the sentinel doesn't work when doing MB matching?

Why do you ask that?

Because if the sentinel worked you wouldn't need to have this comparison. But investigating this can be done after merging.

+      /* If the previous character was a newline, count it. */
+      if (count && (char *) p<= end && p[-1] == eol)
+       ++*count;

[BTW, your mail client seems to be mangling the spacing of quoted code.]

Don't mention it.

When P == end, we want to know if P[-1] (the last byte)
is a newline.  Similarly for any smaller P.
This eliminates the P == end + 1 case, in which the sentinel
would give a false positive.

Ah, I see now. P has been already been incremented when you get here, so it cannot be equal to BEGIN.

P actually does exceed "end" in some cases.

Exactly, for the same reason.  It has already been incremented.

* src/search.c: Adjust to new dfaexec API.
Now, dfaexec returns a pointer, not an integer,
and the third parameter is END, not buffer size.

I suppose you'd squash 1/3 and 2/3 for bisectability?  Or could the
search.c part be applied already to 1/3?

I'd rather not squash them.  1/3 is already so large...
This is probably a good case for a merge commit: i.e.,
push a branch with those two commits, and then merge that
onto master.  Then we have separate commits, yet the result
is still bisectable.

Unfortunately not, because git bisect goes into merges and tries to bisect the commits of the merged branch too. Wouldn't it work if you moved the search.c part to 1/3, and then change just the sentinel implementation in 2/3?

+         *end = saved_end;                             \

Do we need to restore this or can we treat it as a scratch area?

In grep, we must restore it, since it is the first byte of
the following buffer.  In gawk, it is not currently required.


I happily ACK this patchset then, except for asking you to test with search.c parts moved to 1/3 and commit that if it passes.


reply via email to

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