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

[Jonathan Tan <jonathantanmy@google.com>]

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

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

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