Re: [PATCH v2] patch-ids: document intentional const-casting in patch_id_neq()

[cat@malon.dev]

Hi Junio,

> 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.

>     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