On Mon, Apr 2, 2018 at 4:51 PM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> On Mon, 2 Apr 2018 15:48:52 -0700
> Stefan Beller <sbeller@xxxxxxxxxx> wrote:
>
>> At the time the move coloring was implemented we thought an enum of modes
>> is the best to configure this feature. However as we want to tack on new
>> features, the enum would grow exponentially.
>>
>> Refactor the code such that features are enabled via bits. Currently we can
>> * activate the move detection,
>> * enable the block detection on top, and
>> * enable the dimming inside a block, though this could be done without
>> block detection as well (mode "plain, dimmed")
>
> Firstly, patches 1-4 are obviously correct.
>
> As for this patch, I don't think that using flags is the right way to do
> this.
Now that I redid it another way[1], I have the impression that this was the
right approach, because it allows for a short
if (o->color_moved)
condition. If we treat white spaces separately, then we'd have to
have implications such as:
if (some white space option)
the enum = default if not given explicitely.
which we do not need in case of a flags field.
[1] Keeping the enum around and having an extra variable for the
white space related configuration.
> We are not under any size pressure for struct diff_options, and
> the additional options that we plan to add (color-moved-whitespace-flags
> and ignore-space-delta) can easily be additional fields instead.
The traditional white space flags would want to be a field and occupy
the same bits in that field for ease of implementation, and the new option
would just fit in by picking a new place in the bit field.
Thanks,
Stefan
