Re: [PATCH v4 00/12] Cleanup xdfile_t and xrecord_t in xdiff.

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2025, 22:39 UTC
Message-ID: <xmqq1pnyru1f.fsf@gitster.g>
In-Reply-To: <pull.2048.v4.git.git.1758570701.gitgitgadget@gmail.com>

"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Changes since v3.
>
>  * Address review feedback.
>  * Split the deletion of xdl_get_rec() into 2 commits.

>  * Move NO, YES, MAYBE into xprepare.c, and use bool literals.

The elements of this array is of type "char"; it makes me feel a bit awkward that the code

 * assigns bool "true" or "false" to "char"

 * expects that reading it in another compilation unit yields 0 or 1

Instead of abusing boolean true/false, I'd rather see the code that assigns 0 or 1 to use 0 or 1 as literals. As the array got a much better name .changed[], anybody would understand that

    env->xdf.changed[line] = 0;
    env->xdf.changed[line] = 1;

mean what they mean.

>  * refactor 'char rchg' to 'bool changed'

Hmph, I am not sure if it is a good idea to pretend that this changed[] array that is more than bool to be a mere bool. An object declared as type _Bool is guaranteed to be only large enough to store the values 0 and 1. Granted that you cannot allocate less than a single bite or make an array of bits in modern architectures, an array of _Bool would likely be byte addressed and if you assign 2 and read it back, you may get 2 back in practice, but I'd rather not to see such a strange code to live in this codebase.

How about

 - rename rchg[] to changed[], which is a very good move;

 - optionally make it unsigned char, not char;

 - the user of changed[] that uses only 0 or 1 and is not even aware
   of that MAYBE thing use 0 or 1;

 - the user of changed[] that has to be aware of that MAYBE state
   use its own NO/YES/MAYBE for readability.

Hmm?