From: Ezekiel Newren Date: Tue, 23 Sep 2025 00:13:58 GMT Subject: Re: [PATCH v4 00/12] Cleanup xdfile_t and xrecord_t in xdiff. Message-ID: In-Reply-To: On Mon, Sep 22, 2025 at 4:39 PM Junio C Hamano wrote: > > "Ezekiel Newren via GitGitGadget" 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? 'changed' is NEVER EVER!!! assigned anything other than 0 or 1 which strictly makes it a bool. It's easy to mistake that because the functions in xprepare.c that deal with NO, YES, and MAYBE are within a few lines of 'changed'. Please re-read xdl_cleanup_records() and xdl_clean_mmatch() very carefully. I will update my commit message to make this more clear.