git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Emily Shaffer <emilyshaffer@google.com>,
	Jeff King <peff@peff.net>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v2 3/4] grep: copy struct in one fell swoop
Date: Tue, 24 Nov 2020 23:53:37 -0800	[thread overview]
Message-ID: <xmqqlfepg9dq.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAN0heSoM+qQe8BdKHVpqhA0RAqzyyL3Qr98G=O8kD504diruCg@mail.gmail.com> ("Martin Ågren"'s message of "Wed, 25 Nov 2020 07:25:07 +0100")

Martin Ågren <martin.agren@gmail.com> writes:

> On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> 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.




  reply	other threads:[~2020-11-25  7:55 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-21 18:31 [PATCH 0/4] grep: retire `init_grep_defaults()` Martin Ågren
2020-11-21 18:31 ` [PATCH 1/4] grep: don't set up a "default" repo for grep Martin Ågren
2020-11-21 18:31 ` [PATCH 2/4] grep: use designated initializers for `grep_defaults` Martin Ågren
2020-11-21 18:31 ` [PATCH 3/4] grep: simplify color setup Martin Ågren
2020-11-21 20:23   ` Jeff King
2020-11-21 20:52     ` Martin Ågren
2020-11-21 22:46     ` Junio C Hamano
2020-11-24  6:54       ` Jeff King
2020-11-21 18:31 ` [PATCH 4/4] MyFirstObjectWalk: drop `init_walken_defaults()` Martin Ågren
2020-11-23 11:03 ` [PATCH 0/4] grep: retire `init_grep_defaults()` Johannes Schindelin
2020-11-24 21:04 ` [PATCH v2 0/4] grep: simplify "grep defaults" handling Martin Ågren
2020-11-24 21:04   ` [PATCH v2 1/4] grep: don't set up a "default" repo for grep Martin Ågren
2020-11-24 21:04   ` [PATCH v2 2/4] grep: use designated initializers for `grep_defaults` Martin Ågren
2020-11-24 21:04   ` [PATCH v2 3/4] grep: copy struct in one fell swoop Martin Ågren
2020-11-24 22:34     ` Junio C Hamano
2020-11-25  6:25       ` Martin Ågren
2020-11-25  7:53         ` Junio C Hamano [this message]
2020-11-26 20:25           ` Martin Ågren
2020-11-24 21:04   ` [PATCH v2 4/4] MyFirstObjectWalk: drop `init_walken_defaults()` Martin Ågren
2020-11-25  9:27   ` [PATCH v2 0/4] grep: simplify "grep defaults" handling Ævar Arnfjörð Bjarmason
2020-11-29 19:52   ` [PATCH v3 " Martin Ågren
2020-11-29 19:52     ` [PATCH v3 1/4] grep: don't set up a "default" repo for grep Martin Ågren
2020-11-29 19:52     ` [PATCH v3 2/4] grep: use designated initializers for `grep_defaults` Martin Ågren
2020-11-29 19:52     ` [PATCH v3 3/4] grep: copy struct in one fell swoop Martin Ågren
2020-11-29 19:52     ` [PATCH v3 4/4] MyFirstObjectWalk: drop `init_walken_defaults()` Martin Ågren
2020-12-01  4:46     ` [PATCH v3 0/4] grep: simplify "grep defaults" handling Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlfepg9dq.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).