Re: [GSoC][PATCH v3 1/7] fsck: add refs check interfaces to interface with fsck error levels

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 18, 2024, 15:25 UTC
Message-ID: <xmqqfrta2ryp.fsf@gitster.g>
In-Reply-To: <ZnFJTdHQcHVagflt@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

Show 21 quoted lines

> On Tue, Jun 18, 2024 at 04:38:07AM -0400, Karthik Nayak wrote:
>> shejialuo <shejialuo@gmail.com> writes:
>> 
>> [snip]
>> 
>> >  struct fsck_options {
>> > +	/*
>> > +	 * Reorder the fields to allow `fsck_ref_options` to use
>> > +	 * the interfaces using `struct fsck_options`.
>> > +	 */
>> 
>> Why is this added? It makes sense to have it in the commit message
>> because it talks about the change, but why make it persistent in the
>> code?
>> 
>
> I explicitly add this comments due to the following reasons:
>
> 1. If someone needs to change the `fsck_options`, without this comment,
> he might be just add some new fields at top. Although the change will
> fail the tests here, I think we should mention this in code.

Do you mean you plan to take advantage of the fact that early members of two structures are the same? IOW, if there is a function that takes a pointer to smaller fsck_refs_options, you plan to pass a pointer to fsck_options from some callers, e.g.

    extern void func(struct fsck_refs_options *);
    void a_caller(struct fsck_options *o)
    {
	func((struct fsck_options *)o);
	...

If that is the case, then ...

Do not do that.

Your data structure design is broken.  Instead you would do this:

	struct fsck_options {
		struct fsck_refs_options refs;
		... other members ...
	};
	void a_caller(struct fsck_options *o)
	{
		func(&o->refs);
		...