Re: [PATCH 1/3] grep: make pcre1_tables version agnostic

[Carlo Arenas <carenas@gmail.com>]

On Sat, Jul 27, 2019 at 4:47 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Sat, Jul 27 2019, Carlo Marcelo Arenas Belón wrote:
>
> > 6d4b5747f0 ("grep: change internal *pcre* variable & function names
> > to be *pcre1*", 2017-05-25), renamed most variables to be PCRE1
> > specific to give space to similarly named variables for PCRE2, but
> > in this case the change wasn't needed as the types were compatible
> > enough (const unsigned char* vs const uint8_t*) to be shared.

reading this again, had to admit there is a fair amount of guessing on
intent, but reading commits and the email discussion couldn't come
up with a better explanation.

is the root cause for the bug different?, could it be that the pcre2 API
was misunderstood? and you expected this pointer will be free together
with the context? (as it is done when a cloned context with tables is
used?)

> Both the v1 and v2 functions return const unsigned char *. I don't know
> where I got the uint8_t from. This makes more sense.

the type in PCRE2 is uint8_t, the documentation has a bug[1]
almost every platform git cares for would have unsigned char = uint8_t
though.

> The point of 6d4b5747f0 was not to only split out those variables we
> couldn't get away with re-using. Then I would have later re-used
> e.g. pcre1_jit_on & pcre2_jit_on as just pcre_jit_on. We could also do
> that now.

IMHO pcre_jit_on makes more sense as a bit, with local variables being
used for the pcre*_config() call with the right type.(uint32_t != int)

> I think doing that & this part of the your changes makes things less
> readable. The two code branches we compile with ifdefs are mutually
> exclusive, so having the variables be unique helps with eyeballing /
> reasoning when changing the code.

I thought that too until the typo[2] in the pcre?_jit_on variable (now
in next) kind of
proved us wrong.  Maybe the names are too similar?

either way, would you rather drop this patch and make a replica variable?
should I rebase to next with Reviewed-By tags so that all other changes
in flight that would conflict could be corrected?

Carlo

[1] https://bugs.exim.org/show_bug.cgi?id=2420
[2] https://public-inbox.org/git/20190722181923.21572-1-dev+git@drbeat.li/