threads / patch / 17840

patchUse DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

Subject: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

## tl;dr

8 messages between Feb 17, 2009 and Feb 19, 2009. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Keith Cascio· Feb 17, 2009, 03:26 UTC · lore
Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
 diff.h |    3 +++
 diff.c |   17 ++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)
Show changes to 2 files +13 −7

diff.h, diff.c

diff --git a/diff.h b/diff.h
index 6703a4f..6616877 100644
--- a/diff.h
+++ b/diff.h
@@ -69,6 +69,9 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
 #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
+#define DIFF_XDL_TST(opts, flag)    ((opts)->xdl_opts & XDF_##flag)
+#define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |= XDF_##flag)
+#define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag)
 
 struct diff_options {
 	const char *filter;
diff --git a/diff.c b/diff.c
index 006aa01..ff3624e 100644
--- a/diff.c
+++ b/diff.c
@@ -2567,13 +2567,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 
 	/* xdiff options */
 	else if (!strcmp(arg, "-w") || !strcmp(arg, "--ignore-all-space"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE);
 	else if (!strcmp(arg, "-b") || !strcmp(arg, "--ignore-space-change"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);
 	else if (!strcmp(arg, "--ignore-space-at-eol"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
 	else if (!strcmp(arg, "--patience"))
-		options->xdl_opts |= XDF_PATIENCE_DIFF;
+		DIFF_XDL_SET(options, PATIENCE_DIFF);
 
 	/* flags options */
 	else if (!strcmp(arg, "--binary")) {
@@ -2594,10 +2594,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_SET(options, COLOR_DIFF);
 	else if (!strcmp(arg, "--no-color"))
 		DIFF_OPT_CLR(options, COLOR_DIFF);
-	else if (!strcmp(arg, "--color-words"))
-		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
+	else if (!strcmp(arg, "--color-words")) {
+		DIFF_OPT_SET(options, COLOR_DIFF);
+		DIFF_OPT_SET(options, COLOR_DIFF_WORDS);
+	}
 	else if (!prefixcmp(arg, "--color-words=")) {
-		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
+		DIFF_OPT_SET(options, COLOR_DIFF);
+		DIFF_OPT_SET(options, COLOR_DIFF_WORDS);
 		options->word_regex = arg + 14;
 	}
 	else if (!strcmp(arg, "--exit-code"))

-- 
1.6.1
Johannes Schindelin· Feb 17, 2009, 12:05 UTC · re: Keith Cascio · lore

Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

Hi,
On Mon, 16 Feb 2009, Keith Cascio wrote:
> 
> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
> ---
Rationale?

If you are going to add something on top of that, I can understand that, but this patch is not labeled [1/n]. And...

>  diff.h |    3 +++
>  diff.c |   17 ++++++++++-------
>  2 files changed, 13 insertions(+), 7 deletions(-)

... this does not look good to me, without a compelling reason why we want to have the patch nevertheless.

Ciao, Dscho

Keith Cascio· Feb 17, 2009, 17:33 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

Dscho,
On Tue, 17 Feb 2009, Johannes Schindelin wrote:
> Rationale?
> If you are going to add something on top of that, I can understand that, but 
> this patch is not labeled [1/n].  And...

This patch is about consistency. Yes I'd like to add something on top of it. But these improvements stand well on their own. My work on the diff.defaultOptions patch highlighted the desirability of handling these bit manipulations consistently, via macros.

> ... this does not look good to me, without a compelling reason why we want to 
> have the patch nevertheless.
Is there something you dislike about the code style?  As always I'm happy to 
adjust it.
                                    -- Keith
Johannes Schindelin· Feb 17, 2009, 22:57 UTC · re: Keith Cascio · lore

Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

Hi,
On Tue, 17 Feb 2009, Keith Cascio wrote:
> On Tue, 17 Feb 2009, Johannes Schindelin wrote:
> 
> But these improvements stand well on their own.

That was my point; without anything on top I would not like to risk regressions that easily.

Show 5 quoted lines
> > ... this does not look good to me, without a compelling reason why we 
> > want to have the patch nevertheless.
> 
> Is there something you dislike about the code style?  As always I'm 
> happy to adjust it.

If you had not so conveniently clipped what I quoted just before the three dots, I could point out that it adds roughly double the number of lines as it removes.

Ciao, Dscho

Keith Cascio· Feb 18, 2009, 20:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

On Tue, 17 Feb 2009, Johannes Schindelin wrote:
Show 7 quoted lines
> > >  diff.h |    3 +++
> > >  diff.c |   17 ++++++++++-------
> > >  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> If you had not so conveniently clipped what I quoted just before the three 
> dots, I could point out that it adds roughly double the number of lines as it 
> removes.
Didn't they say on Mount Sinai?
"Thou shalt not judge a patch on the diffstat alone."
Johannes Schindelin· Feb 18, 2009, 21:22 UTC · re: Keith Cascio · lore

Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

Hi,
On Wed, 18 Feb 2009, Keith Cascio wrote:
Show 13 quoted lines
> On Tue, 17 Feb 2009, Johannes Schindelin wrote:
> 
> > > >  diff.h |    3 +++
> > > >  diff.c |   17 ++++++++++-------
> > > >  2 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > If you had not so conveniently clipped what I quoted just before the 
> > three dots, I could point out that it adds roughly double the number 
> > of lines as it removes.
> 
> Didn't they say on Mount Sinai?
> 
> "Thou shalt not judge a patch on the diffstat alone."
Okay, you asked for it.  I tried to be gentle.

I see _no_ value in your changes, and the diffstat as a _very_ real downside.

If the code would become clearer with your patch, I would not mind. But I find that the result is not more readable than the original.

As part of a parse-optification, I would not mind.  But before that, no.

Ciao, Dscho

Keith Cascio· Feb 18, 2009, 22:59 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

On Wed, 18 Feb 2009, Johannes Schindelin wrote:
Show 8 quoted lines
> Okay, you asked for it.  I tried to be gentle.
> 
> I see _no_ value in your changes, and the diffstat as a _very_ real downside.
> 
> If the code would become clearer with your patch, I would not mind.  But I 
> find that the result is not more readable than the original.
> 
> As part of a parse-optification, I would not mind.  But before that, no.

Actually I appreciate your feedback, and the more direct the better. The chief value I see in revising the code to accomplish these bit settings uniformly using well known macros is that later, if someone has good reason to extend the macro so it results in some new side effect, e.g. to update a dirty bit mask, the new behavior automatically cascades to every appropriate use out there in the code. A macro is essentially a "code constant". As with other constants, the benefit comes from defining it once and using it everywhere. Is this effect not worth as much as I think it is? Is there a hidden gotcha in my ideal? Or does anyone else see value here? Please speak up!

                                             -- Keith
Jeff King· Feb 19, 2009, 01:02 UTC · re: Keith Cascio · lore

Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

On Wed, Feb 18, 2009 at 02:59:29PM -0800, Keith Cascio wrote:
Show 11 quoted lines
> Actually I appreciate your feedback, and the more direct the better.
> The chief value I see in revising the code to accomplish these bit
> settings uniformly using well known macros is that later, if someone
> has good reason to extend the macro so it results in some new side
> effect, e.g. to update a dirty bit mask, the new behavior
> automatically cascades to every appropriate use out there in the code.
> A macro is essentially a "code constant".  As with other constants,
> the benefit comes from defining it once and using it everywhere.  Is
> this effect not worth as much as I think it is?  Is there a hidden
> gotcha in my ideal?  Or does anyone else see value here?  Please speak
> up!

I think you are running into "if it's not broken, don't fix it". That is, there is a transaction cost to making changes to the code. It can be as small a cost as "now somebody working in a related area has a conflict (even if textual) and has to spend time resolving". Or it can be as big as "you introduced new bugs". In this case, I think any costs would tend towards the former and not the latter.

That cost has to be weighed against the benefit.

The usual attitude in git is that minor cleanups or enhancements like this should be coupled with patches that build on them. Saying "this might help in the future if somebody does X" means you are paying the cost now, but there may or may not ever _be_ any benefit.

In this case, I think there are actually two changes in your patch:
  1. consistently using DIFF_OPT_* macros, which are used in 99% of
     callsites already
  2. adding and using DIFF_XDL macros

I think (1) has immediate value. Anyone looking at the code and seeing the existence of DIFF_OPT macros and how commonly they are used might think they are used exclusively. And that means it is easy to miss callsites when searching through the code. So to me, making things consistent brings value.

For (2), that situation does not exist, since you are introducing new macros.

Personally, I don't mind the abstraction layer of the XDL macros, especially since they match the style of the existing DIFF_OPT macros (and the only reason they are not in the DIFF_OPT bitfield at all is that they must be passed separately to xdiff).

-Peff

← back to recent threads