Re: [PATCH v2] patch-ids: document intentional const-casting in patch_id_neq()
- From
cat@malon.dev <cat@malon.dev>
- Date
- Mar 9, 2026, 06:39 UTC
- Message-ID
- <4d93dbf55e141460989f21edad24440d@purelymail.com>
- In-Reply-To
- <xmqqh5qp97bd.fsf@gitster.g>
Hi Junio,
Show 23 quoted lines
> Is that a "performance regression", I have to wonder? We would > regress relative to what by doing what? > > Is the lazy evaluation avoiding unnecessary work? > > If we are going to pass _all_ the objects in the hashmap to this > comparator function eventually _anyway_, then the total cost of > computing patch IDs to all of them in the hashmap would not change > with or without lazy computation, but if we are currently getting > away without having to compute for all, but only computing for the > ones we pass to this function, then lazy evaluation is clearly a > win. I do not offhand know which of the above two is the case, but > we need to know that before we can touch the NEEDSWORK comment, I > think. > > The lazy computation comes from b3dfeebb (rebase: avoid computing > unnecessary patch IDs, 2016-07-29), even though the "const > correctness?" comment is a bit newer than that. > > So it seems that we indeed are avoiding unnecessary work without > this patch. We'd encounter "performance regression" only if we stop > avoiding unnecessary work, so I am afraid that the phrasing used in > the patch is somewhat confusing.
You're right. Avoiding unnecessary work is indeed a more fundamental reason than preventing performance regression.
Show 9 quoted lines
> Even though eptr and entry_or_key are const, we want to lazily > compute their .patch_id members; see b3dfeebb (rebase: avoid > computing unnecessary patch IDs, 2016-07-29), so cast the > constness away with container_of(). > > or something, perhaps? > >> struct diff_options *opt = (void *)cmpfn_data; >> struct patch_id *a, *b;
I will incorporate your suggested phrasing and reference to the historical commit in v3.
Regards,
Yuchen