git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
May 15, 2017, 02:43 UTC
Message-ID
<xmqqtw4mx2kj.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<CACBZZX4OFermQ3jEQOgozP-H+uZauJj6GEdwKZjqs8CF8xHEDQ@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 38 quoted lines
>> 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!
>
> ...and if I do change it do others think this is something that
> warrants a comment & some whitespace padding? I.e.:
>
> @@ -378,8 +392,17 @@ static int pcre1match(struct grep_pat *p, const
> char *line, const char *eol,
>         if (eflags & REG_NOTBOL)
>                 flags |= PCRE_NOTBOL;
>
> +#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
> +       /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */
>         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) {

If we MUST have this #ifdef/#endif in-line in this function, then tolerating funny indentation in the else clause I think is an accepted common practice that may not need an extra comment.

But I wonder if the resulting code of this function may become simpler to follow if we remove #ifdef/#endif from it, and instead have this function call a helper (which may itself have #ifdef, or maybe #ifdef/#else/#endif may have two implementations)?

Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 9 of 12 in “PCRE v2, PCRE v1 JIT, log -P & fixes”
  1. 0/7 PCRE v2, PCRE v1 JIT, log -P & fixesÆvar Arnfjörð Bjarmason, May 13, 2017
  2. 1/7 grep: don't redundantly compile throwaway patterns under threadingÆvar Arnfjörð Bjarmason, May 13, 2017
  3. 2/7 grep: skip pthreads overhead when using one threadÆvar Arnfjörð Bjarmason, May 13, 2017
  4. 3/7 log: add -P as a synonym for --perl-regexpÆvar Arnfjörð Bjarmason, May 13, 2017
  5. 4/7 grep: add support for the PCRE v1 JIT APIÆvar Arnfjörð Bjarmason, May 13, 2017
  6. Simon RuderichMay 14, 2017
  7. Ævar Arnfjörð BjarmasonMay 14, 2017
  8. Ævar Arnfjörð BjarmasonMay 14, 2017
  9. Junio C HamanoMay 15, 2017
  10. 6/7 grep: un-break building with PCRE < 8.20Ævar Arnfjörð Bjarmason, May 13, 2017
  11. 5/7 grep: un-break building with PCRE < 8.32Ævar Arnfjörð Bjarmason, May 13, 2017
  12. 7/7 grep: add support for PCRE v2Ævar Arnfjörð Bjarmason, May 13, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.