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.

Thanks!

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.

Paolo


Reply via email to