On Sun, 26 Jul 2015 09:10:05 -0700
Jim Meyering <j...@meyering.net> 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 <j...@meyering.net> 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.
