From: Junio C Hamano Date: Mon, 22 Sep 2025 22:39:40 GMT Subject: Re: [PATCH v4 00/12] Cleanup xdfile_t and xrecord_t in xdiff. Message-ID: In-Reply-To: "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?