bug#19306: [PATCH 1/2] dfa: avoid execution for a pattern including an unsupported expression

[Norihiro Tanaka]

On Sun, 26 Jul 2015 09:10:05 -0700
Jim Meyering <address@hidden> wrote:

> You're right. That covers it.
> 
> This patch is good also for eliminating some technical debt.
> Since your change eliminates the code below (effectively making
> the same change from conjunct to disjunct), there is no reason
> to make the following correction:
> 
>                /* Falling back to the glibc matcher in this case gives   \
>                   better performance (up to 25% better on [a-z], for     \
>                   example) and enables support for collating symbols and \
>                   equivalence classes.  */                               \
> -              if (d->states[s].has_mbcset && backref)                   \
> +              if (d->states[s].has_mbcset || backref)                   \
>                  {                                                       \
>                    *backref = 1;                                         \
>                    goto done;                                            \
>                  }                                                       \
> 
> At first I was surprised to see that using "&&" there provoked
> no test failure, but then saw that we test "backref" shortly thereafter.
> While technically, using "&&" there is a bug, it seems the effect was
> negligible.
> 
> I have made some changes, renaming functions, e.g., dfabackref -> 
> dfa_supported,
> since even before this change "backref" meant more than the presence
> of a backreference.
> 
> Note that while your commit log entry described new functions, I have
> modified the commit
> log to say merely "new function" for each. Instead, I document the new
> functions in the code,
> where that documentation will be more useful, and maintained.
> 
> Please let me know if there is anything you'd like to change:

Thanks for reviewing.  

On Sun, 26 Jul 2015 09:10:05 -0700
Jim Meyering <address@hidden> wrote:

> This patch is good also for eliminating some technical debt.
> Since your change eliminates the code below (effectively making
> the same change from conjunct to disjunct), there is no reason
> to make the following correction:
>
>                /* Falling back to the glibc matcher in this case gives   \
>                   better performance (up to 25% better on [a-z], for     \
>                   example) and enables support for collating symbols and \
>                   equivalence classes.  */                               \
> -              if (d->states[s].has_mbcset && backref)                   \
> +              if (d->states[s].has_mbcset || backref)                   \
>                  {                                                       \
>                    *backref = 1;                                         \
>                    goto done;                                            \
>                  }                                                       \
> 
> At first I was surprised to see that using "&&" there provoked
> no test failure, but then saw that we test "backref" shortly thereafter.
> While technically, using "&&" there is a bug, it seems the effect was
> negligible.

The change is wrong, and previous code is right.  If BACKREF is null,
may be core dumped at *BACKREF = 1.

> I have made some changes, renaming functions, e.g., dfabackref -> 
> dfa_supported,
> since even before this change "backref" meant more than the presence
> of a backreference.

I agree.

By the way, now BACKREF is used in meaning of non-support already.  So
In the near future, Maybe it should be changed into UNSUPPORTED etc.

> Note that while your commit log entry described new functions, I have
> modified the commit
> log to say merely "new function" for each. Instead, I document the new
> functions in the code,
> where that documentation will be more useful, and maintained.
> 
> Please let me know if there is anything you'd like to change:

Thanks for ajustment of commit log.  I have no request to change.  I
agree to the changes.