On Sun, May 14, 2017 at 4:43 PM, Simon Ruderich <simon@xxxxxxxxxxxx> wrote:
> 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!