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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 6, 2018, 22:27 UTC
Message-ID: <20180406152734.b36d1301fb49fb6db7307b51@google.com>
In-Reply-To: <CAGZ79kZe1SEy2PkYGe200KwZHZVieAJJaiWYLD7GadOCCcmeOg@mail.gmail.com>

On Fri, 6 Apr 2018 14:28:40 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 13 quoted lines

> 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.

Ah, I think I see what you mean. In your way, move detection can be activated either by explicitly setting a color-move pattern (e.g. zebra), or by setting a move-detection whitespace option, both of which will set bits in the uint32.

As opposed to my proposed way, where you either have to set the default explicitly (like you describe), or write "if (o->color_moved || o->color_moved_whitespace_handling)" instead of "if (o->color_moved)".

I don't think such an implicit dependence (whitespace option to color-move pattern) is reason enough to use a bitfield, and I think the opposite actually - this dependence should be in fact explicit, not implicit. But I'm open to opinions from others.

Show 7 quoted lines

> > 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.

If we were to use bitfields, this would be the way, yes.