Re: [PATCH 1/1] pcre2: allow overriding the system allocator

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2019, 21:53 UTC
Message-ID: <xmqq36ifmg7v.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com>

Carlo Arenas <carenas@gmail.com> writes:

Show 20 quoted lines

> LGTM except from the suggestion below that might make the code more "standard"
> and probably be a good base for a similar PCRE1 fix
>>
>> +static pcre2_general_context *get_pcre2_context(void)
>> +{
>> +       static pcre2_general_context *context;
>> +
>> +       if (!context)
>> +               context = pcre2_general_context_create(pcre2_malloc,
>> +                                                      pcre2_free, NULL);
>> +
>> +       return context;
>> +}
>
> instead of using a static variable inside this helper function it
> might be better to use
> one extra field inside the (struct grep_pat *p), where all other
> variables are kept
>
> Additionally to being more consistent will avoid creating the global

"standard" and "more consistent" are good things, but I am not sure I should agree with the argument without knowing what you are comparing your suggested improvement with. Whose standard practice are we trying to be consistent with? Keeping dynamic resources hooked to "struct grep_pat" so that (1) different patterns could use different settings when they desire and (2) the resources are not hidden behind a function-scope static and can be discarded when we are done with the pattern, which is the standard in our "grep" subsystem?

I think general context probably corresponds to a bit higher level than individual grep_pat. E.g. when running "grep -e foo -e bar", do we expect resources needed by patterns "foo" and "bar" would want to be allocated and freed by potentially separate <alloc,free> function pairs?

> context for the
> most common case (when the locale is either C/POSIX or UTF-8) and therefore
> have a smaller impact on performance.

I am not sure about the impact on performance, but if it helps us keep the subsystem reusable by avoiding function-scope static that we cannot clear, that would be a good thing. But "struct grep_pat" may be a bit too fine-grained to control general context.