Hi Ævar, On Wed, 26 Jun 2019, Ævar Arnfjörð Bjarmason wrote: > Bring back optimized fixed-string search for "grep", this time with > PCRE v2 as an optional backend. As noted in [1] with kwset we were > slower than PCRE v1 and v2 JIT with the kwset backend, so that > optimization was counterproductive. > > This brings back the optimization for "-F", without changing the > semantics of "\0" in patterns. As seen in previous commits in this > series we could support it now, but I'd rather just leave that > edge-case aside so the tests don't need to do one thing or the other > depending on what --fixed-strings backend we're using. Nice. Very, very nice. > I could also support the v1 backend here, but that would make the code > more complex, and I'd rather aim for simplicity here and in future > changes to the diffcore. We're not going to have someone who > absolutely must have faster search, but for whom building PCRE v2 > isn't acceptable. I could not agree more. > diff --git a/grep.c b/grep.c > index 4716217837..6b75d5be68 100644 > --- a/grep.c > +++ b/grep.c > @@ -356,6 +356,18 @@ static NORETURN void compile_regexp_failed(const struct grep_pat *p, > die("%s'%s': %s", where, p->pattern, error); > } > > +static int is_fixed(const char *s, size_t len) > +{ > + size_t i; > + > + for (i = 0; i < len; i++) { > + if (is_regex_special(s[i])) > + return 0; > + } > + > + return 1; > +} > + > #ifdef USE_LIBPCRE1 > static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt) > { > @@ -602,7 +614,6 @@ static int pcre2match(struct grep_pat *p, const char *line, const char *eol, > static void free_pcre2_pattern(struct grep_pat *p) > { > } > -#endif /* !USE_LIBPCRE2 */ Huh? Removing an `#endif` without removing the corresponding `#if`? ... but... > static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt) > { > @@ -623,11 +634,13 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt) > compile_regexp_failed(p, errbuf); > } > } > +#endif /* !USE_LIBPCRE2 */ Ah hah! If we would not have plenty of exercise for the PCRE2 build options, I would be worried. But AFAICT the CI build includes this all the time, so we're fine. > static void compile_regexp(struct grep_pat *p, struct grep_opt *opt) > { > int err; > int regflags = REG_NEWLINE; > + int pat_is_fixed; > > p->word_regexp = opt->word_regexp; > p->ignore_case = opt->ignore_case; > @@ -636,8 +649,38 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt) > if (memchr(p->pattern, 0, p->patternlen) && !opt->pcre2) > die(_("given pattern contains NULL byte (via -f ). This is only supported with -P under PCRE v2")); > > - if (opt->fixed) { > + pat_is_fixed = is_fixed(p->pattern, p->patternlen); > + if (opt->fixed || pat_is_fixed) { > +#ifdef USE_LIBPCRE2 > + opt->pcre2 = 1; > + if (pat_is_fixed) { > + compile_pcre2_pattern(p, opt); > + } else { > + /* > + * E.g. t7811-grep-open.sh relies on the > + * pattern being restored, and unfortunately > + * there's no PCRE compile flag for "this is > + * fixed", so we need to munge it to > + * "\Q\E". > + */ > + char *old_pattern = p->pattern; > + size_t old_patternlen = p->patternlen; > + struct strbuf sb = STRBUF_INIT; > + > + strbuf_add(&sb, "\\Q", 2); > + strbuf_add(&sb, p->pattern, p->patternlen); > + strbuf_add(&sb, "\\E", 2); > + > + p->pattern = sb.buf; > + p->patternlen = sb.len; > + compile_pcre2_pattern(p, opt); > + p->pattern = old_pattern; > + p->patternlen = old_patternlen; > + strbuf_release(&sb); > + } > +#else > compile_fixed_regexp(p, opt); > +#endif It might be a bit easier to read if the shorter clause came first. Other than that: what a nice read. I should save reviewing all your patch series for just-before-bed time. Thanks, Dscho > return; > } > > -- > 2.22.0.455.g172b71a6c5 > >