Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code

From: Stefan Beller <sbeller@google.com>
Date: Apr 6, 2018, 21:28 UTC
Message-ID: <CAGZ79kZe1SEy2PkYGe200KwZHZVieAJJaiWYLD7GadOCCcmeOg@mail.gmail.com>
In-Reply-To: <20180402165144.73e701d7816ddfcf00006d95@google.com>

On Mon, Apr 2, 2018 at 4:51 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

Show 17 quoted lines

> On Mon,  2 Apr 2018 15:48:52 -0700
> Stefan Beller <sbeller@google.com> 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