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

[Junio C Hamano <gitster@pobox.com>]

Tian Yuchen <cat@malon.dev> writes:

> +	/*
> +	 * We drop the 'const' modifier here intentionally.
> +	 *
> +	 * The hashmap API requires us to treat the entries as const.
> +	 * However, to avoid performance regression, we lazily compute
> +	 * the patch IDs inside this comparison function. This fundamentally
> +	 * requires us to mutate the 'struct patch_id'. Therefore, we use
> +	 * container_of() to cast away the constness from the hashmap_entry.
> +	 */

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.

    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;