From: Andrzej Hunt <andrzej@ahunt.org>
To: "René Scharfe" <l.s.r@web.de>,
"Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>, Andrzej Hunt <ajrhunt@google.com>
Subject: Re: [PATCH] merge-ort: only do pointer arithmetic for non-empty lists
Date: Sun, 11 Apr 2021 11:12:21 +0200 [thread overview]
Message-ID: <a5e9bbd6-23bb-2778-2f2a-dc601c842336@ahunt.org> (raw)
In-Reply-To: <1866b90b-fe07-18df-0d60-e2350d935375@web.de>
On 10/04/2021 13:48, René Scharfe wrote:
> Am 10.04.21 um 10:30 schrieb Andrzej Hunt via GitGitGadget:
>> [...]
>> diff --git a/merge-ort.c b/merge-ort.c
>> index 5e118a85ee04..4da4b4688336 100644
>> --- a/merge-ort.c
>> +++ b/merge-ort.c
>> @@ -2504,8 +2504,10 @@ static void write_tree(struct object_id *result_oid,
>> * We won't use relevant_entries again and will let it just pop off the
>> * stack, so there won't be allocation worries or anything.
>> */
>> - relevant_entries.items = versions->items + offset;
>> - relevant_entries.nr = versions->nr - offset;
>> + if (versions->nr) {
>> + relevant_entries.items = versions->items + offset;
>> + relevant_entries.nr = versions->nr - offset;
>> + }
>> QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);
>
> Reading the diff I was wondering if QSORT now gets handed uninitialized
> values if version-nr is 0. The answer is no -- relevant_entries is
> initialized at declaration. Otherwise the compiler would have probably
> warned, but sometimes it gets confused.
>
> I wonder why relevant_entries is introduced at all, though. It's not
> referenced later. So how about this instead?
>
> if (versions->nr)
> QSORT(versions->items + offset, nr, tree_entry_order);
>
> The intent to sort the last versions->nr-offset entries of versions,
> but only if it's not empty, is easier to see like this, I think.
>
That is much more elegant, I will follow this approach. Thank you for
the suggestion!
An alternative might be to keep relevant_entries, and replace all later
usages of versions->items[offset+i] with relevant_entries.items[i], but
that's more invasive and I don't see any good reason for doing so given
that the existing pattern works fine.
next prev parent reply other threads:[~2021-04-11 9:12 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-10 8:30 [PATCH] merge-ort: only do pointer arithmetic for non-empty lists Andrzej Hunt via GitGitGadget
2021-04-10 11:48 ` René Scharfe
2021-04-10 22:56 ` Junio C Hamano
2021-04-11 9:14 ` Andrzej Hunt
2021-04-11 9:12 ` Andrzej Hunt [this message]
2021-04-11 11:05 ` [PATCH v2] " Andrzej Hunt via GitGitGadget
2021-04-12 15:52 ` Elijah Newren
2021-04-12 17:39 ` Junio C Hamano
2021-04-12 17:47 ` 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a5e9bbd6-23bb-2778-2f2a-dc601c842336@ahunt.org \
--to=andrzej@ahunt.org \
--cc=ajrhunt@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=l.s.r@web.de \
--cc=newren@gmail.com \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.