Martin Ågren <martin.agren@xxxxxxxxx> writes:
> On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> +/*
>> + * grep_config() initializes one "default" instance of this type, and
>> + * it is copied by grep_init() to be used by each individual
>> + * invocation. When adding a new field to this structure that is
>> + * populated from the configuration, be sure to think about ownership
>> + * (i.e. a shallow copy may not be what you want for the type of your
>> + * newly added field).
>> + */
>> struct grep_opt {
>> struct grep_pat *pattern_list;
>> struct grep_pat **pattern_tail;
>
> Ok, that makes sense. Maybe put it in `grep_config()` though? We can add
> anything we want to to this struct and initialize it from the command
> line. It's when we start pre-filling it in `grep_config()` that we need
> to think about this. What do you think? We could also do both of
> course to really hedge our bets...
I agree with you that it would be the most helpful to have the
comment near grep_config(), as that function is what defines the
design of populating the singleton to be copied by the instance
used by individual invocation.
> /*
> * The instance of grep_opt that we set up here is copied by
> * grep_init() to be used by each individual invocation.
> * When populating a new field of this structure here,
> * be sure to think about ownership (i.e. a shallow copy in
> * grep_init() may not be what you want).
> */
I find the text near the end of both my version and yours a bit
unsatisfying. One thing I care about is not to mislead readers to
think that the way grep_init() copies the singleton template is
correct and sacred and they need to design their data structure to
be compatible with the shallow copying. We'd want it to be clear
that it is expected that they will deep copy the field, and release
it once individual invocation is done, when they need a new field
that won't work well with shallow copying. Perhaps "may not be wnat
you want" is explicit enough, but I dunno.
Thanks.
