From: Junio C Hamano Date: Sun, 08 Mar 2026 06:26:18 GMT Subject: Re: [PATCH] patch-ids: achieve const correctness in patch_id_neq() Message-ID: In-Reply-To: <20260308043131.77782-1-a3205153416@gmail.com> Tian Yuchen 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.