From: Tian Yuchen <a3205153416@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] patch-ids: achieve const correctness in patch_id_neq()
Date: Sun, 8 Mar 2026 22:42:43 +0800 [thread overview]
Message-ID: <1a1ed5c6-8843-4bd5-9f57-187ef39497c3@gmail.com> (raw)
In-Reply-To: <xmqqseaasuph.fsf@gitster.g>
Hi Junio,
On 3/8/26 14:26, Junio C Hamano wrote:
> The fact that `patch_id_neq` receives `const struct hashmap_entry *`
> parameters--which is a requirement of the `hashmap` API--is what is
> clashing with our lazy initialization here. The original code
> handled this by casting to a non-const `struct patch_id *` right at
> the beginning (via `container_of`). While this also "drops" the
> constness, the original is at least more honest about the fact that
> `a` and `b` will be modified.
Oops, This is something I hadn't considered before. I admit I didn't
think things through carefully enough.
So, we actually find ourselves in this dilemma:
- The Hashmap API specification mandates that input parameters should
be const *in principle*.
- The lazy loading mechanism requires us to write the results into
memory; otherwise, there will be significant performance loss.
Am I correct?
I find that maintaining the current approach seems the most reasonable
option. Computing all patch ids before putting objects into the hashmap
appears to be a move that affects everything else and is not worth the
effort. On the contrary, slightly breaking the Hashmap API conventions
seems to be a more *cost-effective* approach...
Or perhaps it would be more reasonable to slightly modify this NEEDSWORK
flag here?
Will send the next patch shortly.
> If we truly wanted to achieve const-correctness here, we would
> likely need to avoid lazy initialization within the comparison
> function altogether, pre-calculating the full patch ID before it's
> needed for comparison. The NEEDSWORK comment should remain until a
> more fundamental solution is found, or if we should just admit that
> the current lazy evaluation pattern is what we want and document
> that (i.e., add a comment to justify why we strip away the constness
> here). As it stands, this patch doesn't "ensure" const correctness;
> it just masks the violation with an explicit cast at the location
> the pointer is used.
Regards,
Yuchen
next prev parent reply other threads:[~2026-03-08 14:42 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-08 4:31 [PATCH] patch-ids: achieve const correctness in patch_id_neq() Tian Yuchen
2026-03-08 6:26 ` Junio C Hamano
2026-03-08 14:42 ` Tian Yuchen [this message]
2026-03-08 15:02 ` [PATCH v2] patch-ids: document intentional const-casting " Tian Yuchen
2026-03-09 0:26 ` Junio C Hamano
2026-03-09 6:39 ` cat
2026-03-09 6:51 ` [PATCH v3] " Tian Yuchen
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=1a1ed5c6-8843-4bd5-9f57-187ef39497c3@gmail.com \
--to=a3205153416@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox