Re: [PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits

[Junio C Hamano]

Johannes Schindelin  writes:

>> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
>> index b090ad8eac..cbf5d8e166 100644
>> --- a/xdiff/xdiff.h
>> +++ b/xdiff/xdiff.h
>> @@ -27,22 +27,26 @@
>>  extern "C" {
>>  #endif /* #ifdef __cplusplus */
>>  
>> +/* xpparm_t.flags */
>> +#define XDF_NEED_MINIMAL (1 << 0)
>>  
>> -#define XDF_NEED_MINIMAL (1 << 1)
>
> This change makes me wonder whether the least significant bit was omitted
> on purpose originally. You probably looked at that? May be worth
> mentioning in the commit message.
>
>> -#define XDF_IGNORE_WHITESPACE (1 << 2)
>> -#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>> -#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
>> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | 
>> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
>> +#define XDF_IGNORE_WHITESPACE (1 << 1)
>> +#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)
>> +#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)
>> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \
>> +  XDF_IGNORE_WHITESPACE_CHANGE | \
>> +  XDF_IGNORE_WHITESPACE_AT_EOL)
>>  
>> -#define XDF_PATIENCE_DIFF (1 << 5)
>> -#define XDF_HISTOGRAM_DIFF (1 << 6)
>> +#define XDF_IGNORE_BLANK_LINES (1 << 7)
>> +
>> +#define XDF_PATIENCE_DIFF (1 << 14)
>> +#define XDF_HISTOGRAM_DIFF (1 << 15)
>>  #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
>>  #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
>>  
>> -#define XDF_IGNORE_BLANK_LINES (1 << 7)
>> -
>> -#define XDF_INDENT_HEURISTIC (1 << 8)
>> +#define XDF_INDENT_HEURISTIC (1 << 23)
>>  
>> +/* xdemitconf_t.flags */
>>  #define XDL_EMIT_FUNCNAMES (1 << 0)
>>  #define XDL_EMIT_FUNCCONTEXT (1 << 2)
>
> It is a pity that this diff is not easier to review, but it shows how much
> it was in need of cleaning up. Looks much nicer now.
>
> I wonder, however, what your guiding principle was in determining the
> gaps? I would have expected consecutive bits, except for the one gap to
> make room for the upcoming flag, of course.

There wasn't that deep a thought went into it, actually.  

I wanted to give room for each "groupsof bits" to grow, and gave
each group a byte.  Whitespace bits sits in the first byte (but
ignore-blank-lines work quite different so it grabs the bit from the
other end), fundamental choices of LCS algos is another group that
sits in the second byte, then hunk shifting and other heuristics as
another group.  I guess that the "need minimal" thing could have
been thrown into the last group, but somehow I didn't think it would
grow that much, so I left it at the lowest place.

Re: [PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Nov 2017, Junio C Hamano wrote:

> We have packed the bits too tightly in such a way that it is not
> easy to add a new type of whitespace ignoring option, a new type
> of LCS algorithm, or a new type of post-cleanup heuristics.
> 
> Reorder bits a bit to give room for these three classes of options
> to grow.  Also make use of XDF_WHITESPACE_FLAGS macro where we check
> any of these bits are on, instead of using DIFF_XDL_TST() macro on
> individual possibilities.  That way, the "is any of the bits on?"
> code does not have to change when we add more ways to ignore
> whitespaces.
> 
> While at it, add a comment in front of the bit definitions to
> clarify in which structure these defined bits may appear.

Makes sense.

> diff --git a/diff.c b/diff.c
> index 74283d9001..790250fe86 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3434,9 +3434,7 @@ void diff_setup_done(struct diff_options *options)
>* inside contents.
>*/
>  
> - if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
> - DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
> - DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
> + if ((options->xdl_opts & XDF_WHITESPACE_FLAGS))

Not only shorter now, but a lot clearer, too: nobody needs to wonder now
whether one whitespace flag was excluded on purpose.

>   DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
>   else
>   DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index b090ad8eac..cbf5d8e166 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -27,22 +27,26 @@
>  extern "C" {
>  #endif /* #ifdef __cplusplus */
>  
> +/* xpparm_t.flags */
> +#define XDF_NEED_MINIMAL (1 << 0)
>  
> -#define XDF_NEED_MINIMAL (1 << 1)

This change makes me wonder whether the least significant bit was omitted
on purpose originally. You probably looked at that? May be worth
mentioning in the commit message.

> -#define XDF_IGNORE_WHITESPACE (1 << 2)
> -#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
> -#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | 
> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
> +#define XDF_IGNORE_WHITESPACE (1 << 1)
> +#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)
> +#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)
> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \
> +   XDF_IGNORE_WHITESPACE_CHANGE | \
> +   XDF_IGNORE_WHITESPACE_AT_EOL)
>  
> -#define XDF_PATIENCE_DIFF (1 << 5)
> -#define XDF_HISTOGRAM_DIFF (1 << 6)
> +#define XDF_IGNORE_BLANK_LINES (1 << 7)
> +
> +#define XDF_PATIENCE_DIFF (1 << 14)
> +#define XDF_HISTOGRAM_DIFF (1 << 15)
>  #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
>  #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
>  
> -#define XDF_IGNORE_BLANK_LINES (1 << 7)
> -
> -#define XDF_INDENT_HEURISTIC (1 << 8)
> +#define XDF_INDENT_HEURISTIC (1 << 23)
>  
> +/* xdemitconf_t.flags */
>  #define XDL_EMIT_FUNCNAMES (1 << 0)
>  #define XDL_EMIT_FUNCCONTEXT (1 << 2)

It is a pity that this diff is not easier to review, but it shows how much
it was in need of cleaning up. Looks much nicer now.

I wonder, however, what your guiding principle was in determining the
gaps? I would have expected consecutive bits, except for the one gap to
make room for the upcoming flag, of course.

Future patch series could always start by making room for new flags, as
needed, after all.

Ciao,
Dscho

[PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits

[Junio C Hamano]

We have packed the bits too tightly in such a way that it is not
easy to add a new type of whitespace ignoring option, a new type
of LCS algorithm, or a new type of post-cleanup heuristics.

Reorder bits a bit to give room for these three classes of options
to grow.  Also make use of XDF_WHITESPACE_FLAGS macro where we check
any of these bits are on, instead of using DIFF_XDL_TST() macro on
individual possibilities.  That way, the "is any of the bits on?"
code does not have to change when we add more ways to ignore
whitespaces.

While at it, add a comment in front of the bit definitions to
clarify in which structure these defined bits may appear.

Signed-off-by: Junio C Hamano 
---
 diff.c|  4 +---
 xdiff/xdiff.h | 24 ++--
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/diff.c b/diff.c
index 74283d9001..790250fe86 100644
--- a/diff.c
+++ b/diff.c
@@ -3434,9 +3434,7 @@ void diff_setup_done(struct diff_options *options)
 * inside contents.
 */
 
-   if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
-   DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
-   DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
+   if ((options->xdl_opts & XDF_WHITESPACE_FLAGS))
DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
else
DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index b090ad8eac..cbf5d8e166 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -27,22 +27,26 @@
 extern "C" {
 #endif /* #ifdef __cplusplus */
 
+/* xpparm_t.flags */
+#define XDF_NEED_MINIMAL (1 << 0)
 
-#define XDF_NEED_MINIMAL (1 << 1)
-#define XDF_IGNORE_WHITESPACE (1 << 2)
-#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
-#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
-#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | 
XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
+#define XDF_IGNORE_WHITESPACE (1 << 1)
+#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)
+#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)
+#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \
+ XDF_IGNORE_WHITESPACE_CHANGE | \
+ XDF_IGNORE_WHITESPACE_AT_EOL)
 
-#define XDF_PATIENCE_DIFF (1 << 5)
-#define XDF_HISTOGRAM_DIFF (1 << 6)
+#define XDF_IGNORE_BLANK_LINES (1 << 7)
+
+#define XDF_PATIENCE_DIFF (1 << 14)
+#define XDF_HISTOGRAM_DIFF (1 << 15)
 #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
 #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
 
-#define XDF_IGNORE_BLANK_LINES (1 << 7)
-
-#define XDF_INDENT_HEURISTIC (1 << 8)
+#define XDF_INDENT_HEURISTIC (1 << 23)
 
+/* xdemitconf_t.flags */
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
 
-- 
2.15.0-263-g47cc852023