git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Martin Ågren" <martin.agren@gmail.com>,
	git@vger.kernel.org, "Emily Shaffer" <emilyshaffer@google.com>
Subject: Re: [PATCH 3/4] grep: simplify color setup
Date: Sat, 21 Nov 2020 14:46:14 -0800
Message-ID: <xmqq4klis4zt.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201121202310.GA972561@coredump.intra.peff.net> (Jeff King's message of "Sat, 21 Nov 2020 15:23:10 -0500")

Jeff King <peff@peff.net> writes:

> On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:
>
>> The previous commit left us with only one user of the one-line wrapper
>> `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing
>> all the entries in one array into another array of the same type. We
>> might as well just `memcpy()` everything into place.
>> 
>> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
>> ---
>>  Cc-ing Peff, who initially introduced this helper. After having inlined
>>  the function into the for loop, it seemed better to just copy the whole
>>  array. Happy to hear arguments against.
>
> No, this is way better than the existing code. I introduced it to get
> away from strcpy(), but this is better still. But...

Yes, the copy in this patch looks eminently sensible.

>>  Come to think of it, I suppose we could copy the whole struct and not
>>  just the color array. Hmmm...
>
> Yes, this seems even better. If our goal is just to start our new
> grep_opt the same as grep_defaults, then a single-line struct copy
> (whether through assignment or memcpy) is even clearer and more
> maintainable.

... until such a time when typeof(grep_defaults) gains a field with
a non-const pointer value that we'd rather not to share amongst
instances of the type, at which point it no longer is clear win from
maintainability's point of view.

  parent reply	other threads:[~2020-11-21 22:48 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 [this message]
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
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=xmqq4klis4zt.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git