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

[Junio C Hamano <gitster@pobox.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?