Re: [PATCH] patch-ids: achieve const correctness in patch_id_neq()

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

Tian Yuchen <a3205153416@gmail.com> writes:

> The implementation of the 'contain_of' macro in 'patch_id_neq()' is:
>
> 	#define container_of(ptr, type, member) \
> 		((type *) ((char *)(ptr) - offsetof(type, member)))
>
> Here, 'type' is passed as a raw type with no const information.
> Consequently, const correctness cannot be guaranteed here, resulting
> in an eight-year-long NEEDSWORK comment.
>
> Use explicit casting (struct object_id *) to ensure const correctness.

The "NEEDSWORK: const correctness?" comment in patch-ids.c:patch_id_neq()
is indeed about the fact that this function modifies its arguments `eptr`
and `entry_or_key` (via the `patch_id` pointers `a` and `b`) to lazily
compute the full patch ID.

I am afraid, however, that this patch may not be moving in the right
direction. By marking `a` and `b` as `const struct patch_id *`, you are
telling the compiler and the reader that the objects they point to will
not be modified. But then you immediately cast that constness away when
calling `commit_patch_id()`:

> -	    commit_patch_id(a->commit, opt, &a->patch_id, 0))
> +	    commit_patch_id(a->commit, opt, (struct object_id *)&a->patch_id, 0))

I wonder if this is actually any better than the original code. If an
object is marked `const`, it is generally supposed to be immutable.
Casting away the constness of a member to pass it to a function that is
expected to write its findings into that memory region sounds wrong to me.

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.

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.