Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 14, 2017, 15:10 UTC
Message-ID: <CACBZZX5tL2YJyMgO9dnLzwe0SBPJhdOY4jpnJQBtfVRasS2Xow@mail.gmail.com>
In-Reply-To: <20170514144315.w46t7iggp7yeqn4n@ruderich.org>

On Sun, May 14, 2017 at 4:43 PM, Simon Ruderich <simon@ruderich.org> wrote:

Show 51 quoted lines

> On Sat, May 13, 2017 at 11:45:32PM +0000, Ęvar Arnfjörš Bjarmason wrote:
>> [snip]
>>
>> +#ifdef PCRE_CONFIG_JIT
>> +     if (p->pcre1_jit_on)
>> +             ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>> +                                 eol - line, 0, flags, ovector,
>> +                                 ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>> +     else
>> +             ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>> +                             eol - line, 0, flags, ovector,
>> +                             ARRAY_SIZE(ovector));
>> +#else
>>       ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>>                       0, flags, ovector, ARRAY_SIZE(ovector));
>> +#endif
>
> Wouldn't it be simpler to remove the duplication and
> unconditionally use the old pcre_exec() call? Something like
> this:
>
>     +#ifdef PCRE_CONFIG_JIT
>     +   if (p->pcre1_jit_on)
>     +           ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>     +                               eol - line, 0, flags, ovector,
>     +                               ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>     +   else
>     +#endif
>         ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>                         0, flags, ovector, ARRAY_SIZE(ovector));
>
>>       if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>>               die("pcre_exec failed with error code %d", ret);
>>       if (ret > 0) {
>> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
>>  static void free_pcre1_regexp(struct grep_pat *p)
>>  {
>>       pcre_free(p->pcre1_regexp);
>> +#ifdef PCRE_CONFIG_JIT
>> +     if (p->pcre1_jit_on) {
>> +             pcre_free_study(p->pcre1_extra_info);
>> +             pcre_jit_stack_free(p->pcre1_jit_stack);
>> +     } else {
>> +             pcre_free(p->pcre1_extra_info);
>> +     }
>> +#else
>>       pcre_free(p->pcre1_extra_info);
>> +#endif
>
> Same here. The pcre_free() is the same with and without the
> ifdef.

Yes I could do that, no reason not to, and as you point out it would reduce duplication.

I wrote it like this trying to preserve the indentation with/without the macro being true, thinking someone would have an issue with it otherwise.

I also thought just now that perhaps if it were changed the code like that it would warn under -Wmisleading-indentation, but at least on gcc that's not the case, it knows not to warn in the presence of macros.

Unless someone feel strongly otherwise / can think of a good reason for why not, I'll change it as you suggest in the next version.

Thanks for the review!