[PATCH 1/2] grep: use grep_or_expr() in compile_pattern_or()

[PATCH 1/2] grep: use grep_or_expr() in compile_pattern_or()

[René Scharfe]

Move the definition of grep_or_expr() up and use this function in
compile_pattern_or() to reduce code duplication.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 grep.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/grep.c b/grep.c
index 47c75ab7fb..f1bbe80ccb 100644
--- a/grep.c
+++ b/grep.c
@@ -595,6 +595,15 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 	}
 }

+static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)
+{
+	struct grep_expr *z = xcalloc(1, sizeof(*z));
+	z->node = GREP_NODE_OR;
+	z->u.binary.left = left;
+	z->u.binary.right = right;
+	return z;
+}
+
 static struct grep_expr *compile_pattern_or(struct grep_pat **);
 static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 {
@@ -677,7 +686,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)
 static struct grep_expr *compile_pattern_or(struct grep_pat **list)
 {
 	struct grep_pat *p;
-	struct grep_expr *x, *y, *z;
+	struct grep_expr *x, *y;

 	x = compile_pattern_and(list);
 	p = *list;
@@ -685,11 +694,7 @@ static struct grep_expr *compile_pattern_or(struct grep_pat **list)
 		y = compile_pattern_or(list);
 		if (!y)
 			die("not a pattern expression %s", p->pattern);
-		CALLOC_ARRAY(z, 1);
-		z->node = GREP_NODE_OR;
-		z->u.binary.left = x;
-		z->u.binary.right = y;
-		return z;
+		return grep_or_expr(x, y);
 	}
 	return x;
 }
@@ -714,15 +719,6 @@ static struct grep_expr *grep_true_expr(void)
 	return z;
 }

-static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)
-{
-	struct grep_expr *z = xcalloc(1, sizeof(*z));
-	z->node = GREP_NODE_OR;
-	z->u.binary.left = left;
-	z->u.binary.right = right;
-	return z;
-}
-
 static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 {
 	struct grep_pat *p;
--
2.34.1

[PATCH 2/2] grep: use grep_not_expr() in compile_pattern_not()

[René Scharfe]

Move the definition of grep_not_expr() up and use this function in
compile_pattern_not() to simplify the code and reduce duplication.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 grep.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/grep.c b/grep.c
index f1bbe80ccb..bdbd06d437 100644
--- a/grep.c
+++ b/grep.c
@@ -595,6 +595,14 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 	}
 }

+static struct grep_expr *grep_not_expr(struct grep_expr *expr)
+{
+	struct grep_expr *z = xcalloc(1, sizeof(*z));
+	z->node = GREP_NODE_NOT;
+	z->u.unary = expr;
+	return z;
+}
+
 static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
@@ -647,12 +655,10 @@ static struct grep_expr *compile_pattern_not(struct grep_pat **list)
 		if (!p->next)
 			die("--not not followed by pattern expression");
 		*list = p->next;
-		CALLOC_ARRAY(x, 1);
-		x->node = GREP_NODE_NOT;
-		x->u.unary = compile_pattern_not(list);
-		if (!x->u.unary)
+		x = compile_pattern_not(list);
+		if (!x)
 			die("--not followed by non pattern expression");
-		return x;
+		return grep_not_expr(x);
 	default:
 		return compile_pattern_atom(list);
 	}
@@ -704,14 +710,6 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)
 	return compile_pattern_or(list);
 }

-static struct grep_expr *grep_not_expr(struct grep_expr *expr)
-{
-	struct grep_expr *z = xcalloc(1, sizeof(*z));
-	z->node = GREP_NODE_NOT;
-	z->u.unary = expr;
-	return z;
-}
-
 static struct grep_expr *grep_true_expr(void)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
--
2.34.1

[PATCH 0/2] grep: introduce and use grep_and_expr()

[Taylor Blau]

René,

Here are a couple of extra patches on top of your series which introduce
and use a new grep_and_expr() function.

Like the final patch says, this isn't about reducing code duplication,
but rather about adding visual consistency with the other
`compile_pattern_xyz()` functions.

Taylor Blau (2):
  grep: extract grep_binexp() from grep_or_expr()
  grep: use grep_and_expr() in compile_pattern_and()

 grep.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

--
2.34.1.455.gd6eb6fd089

[PATCH 1/2] grep: extract grep_binexp() from grep_or_expr()

[Taylor Blau]

When constructing an OR node, the grep.c code uses `grep_or_expr()` to
make a node, assign its kind, and set its left and right children. The
same is not done for AND nodes.

Prepare to introduce a new `grep_and_expr()` function which will share
code with the existing implementation of `grep_or_expr()` by introducing
a new function which compiles either kind of binary expression, and
reimplement `grep_or_expr()` in terms of it.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/grep.c b/grep.c
index bdbd06d437..d772fe6cc5 100644
--- a/grep.c
+++ b/grep.c
@@ -603,15 +603,22 @@ static struct grep_expr *grep_not_expr(struct grep_expr *expr)
 	return z;
 }
 
-static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)
+static struct grep_expr *grep_binexp(enum grep_expr_node kind,
+				     struct grep_expr *left,
+				     struct grep_expr *right)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
-	z->node = GREP_NODE_OR;
+	z->node = kind;
 	z->u.binary.left = left;
 	z->u.binary.right = right;
 	return z;
 }
 
+static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)
+{
+	return grep_binexp(GREP_NODE_OR, left, right);
+}
+
 static struct grep_expr *compile_pattern_or(struct grep_pat **);
 static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 {
-- 
2.34.1.455.gd6eb6fd089

[PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and()

[Taylor Blau]

In a similar spirit as a previous commit, use the new `grep_and_expr()`
to construct the AND node in `compile_pattern_and()`.

Unlike the aforementioned previous commit, this is not about code
duplication, since this is the only spot in grep.c where an AND node is
constructed. Rather, this is about visual consistency with the other
`compile_pattern_xyz()` functions.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index d772fe6cc5..ab4fdacaed 100644
--- a/grep.c
+++ b/grep.c
@@ -619,6 +619,11 @@ static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *
 	return grep_binexp(GREP_NODE_OR, left, right);
 }
 
+static struct grep_expr *grep_and_expr(struct grep_expr *left, struct grep_expr *right)
+{
+	return grep_binexp(GREP_NODE_AND, left, right);
+}
+
 static struct grep_expr *compile_pattern_or(struct grep_pat **);
 static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 {
@@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)
 		y = compile_pattern_and(list);
 		if (!y)
 			die("--and not followed by pattern expression");
-		CALLOC_ARRAY(z, 1);
-		z->node = GREP_NODE_AND;
-		z->u.binary.left = x;
-		z->u.binary.right = y;
-		return z;
+		return grep_and_expr(x, y);
 	}
 	return x;
 }
-- 
2.34.1.455.gd6eb6fd089

Re: [PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and()

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> In a similar spirit as a previous commit, use the new `grep_and_expr()`
> to construct the AND node in `compile_pattern_and()`.
>
> Unlike the aforementioned previous commit, this is not about code
> duplication, since this is the only spot in grep.c where an AND node is
> constructed. Rather, this is about visual consistency with the other
> `compile_pattern_xyz()` functions.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  grep.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index d772fe6cc5..ab4fdacaed 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -619,6 +619,11 @@ static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *
>  	return grep_binexp(GREP_NODE_OR, left, right);
>  }
>  
> +static struct grep_expr *grep_and_expr(struct grep_expr *left, struct grep_expr *right)
> +{
> +	return grep_binexp(GREP_NODE_AND, left, right);
> +}
> +
>  static struct grep_expr *compile_pattern_or(struct grep_pat **);
>  static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
>  {
> @@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)
>  		y = compile_pattern_and(list);
>  		if (!y)
>  			die("--and not followed by pattern expression");
> -		CALLOC_ARRAY(z, 1);
> -		z->node = GREP_NODE_AND;
> -		z->u.binary.left = x;
> -		z->u.binary.right = y;
> -		return z;
> +		return grep_and_expr(x, y);

You'd need to remove 'z' from the function to avoid getting yelled
at by your compiler for unused variable.

Re: [PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and()

[Taylor Blau]

On Thu, Jan 06, 2022 at 02:14:47PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > @@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)
> >  		y = compile_pattern_and(list);
> >  		if (!y)
> >  			die("--and not followed by pattern expression");
> > -		CALLOC_ARRAY(z, 1);
> > -		z->node = GREP_NODE_AND;
> > -		z->u.binary.left = x;
> > -		z->u.binary.right = y;
> > -		return z;
> > +		return grep_and_expr(x, y);
>
> You'd need to remove 'z' from the function to avoid getting yelled
> at by your compiler for unused variable.

How embarrassing :-). Thanks for noticing, I wrote this so hastily I
neglected to even compile it with DEVELOPER=1.

The new version I sent in response fixes this issue.

Thanks,
Taylor

[PATCH v2 0/2] grep: introduce and use grep_and_expr()

[Taylor Blau]

This small reroll fixes an issue in the second patch where a stack
variable was left unused.

Taylor Blau (2):
  grep: extract grep_binexp() from grep_or_expr()
  grep: use grep_and_expr() in compile_pattern_and()

 grep.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  dae476d1bd grep: extract grep_binexp() from grep_or_expr()
1:  71bd2d1bcc ! 2:  f637e02422 grep: use grep_and_expr() in compile_pattern_and()
    @@ grep.c: static struct grep_expr *grep_or_expr(struct grep_expr *left, struct gre
      static struct grep_expr *compile_pattern_or(struct grep_pat **);
      static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
      {
    +@@ grep.c: static struct grep_expr *compile_pattern_not(struct grep_pat **list)
    + static struct grep_expr *compile_pattern_and(struct grep_pat **list)
    + {
    + 	struct grep_pat *p;
    +-	struct grep_expr *x, *y, *z;
    ++	struct grep_expr *x, *y;
    +
    + 	x = compile_pattern_not(list);
    + 	p = *list;
     @@ grep.c: static struct grep_expr *compile_pattern_and(struct grep_pat **list)
      		y = compile_pattern_and(list);
      		if (!y)
--
2.34.1.455.gd6eb6fd089

[PATCH v2 1/2] grep: extract grep_binexp() from grep_or_expr()

[Taylor Blau]

When constructing an OR node, the grep.c code uses `grep_or_expr()` to
make a node, assign its kind, and set its left and right children. The
same is not done for AND nodes.

Prepare to introduce a new `grep_and_expr()` function which will share
code with the existing implementation of `grep_or_expr()` by introducing
a new function which compiles either kind of binary expression, and
reimplement `grep_or_expr()` in terms of it.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/grep.c b/grep.c
index bdbd06d437..d772fe6cc5 100644
--- a/grep.c
+++ b/grep.c
@@ -603,15 +603,22 @@ static struct grep_expr *grep_not_expr(struct grep_expr *expr)
 	return z;
 }
 
-static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)
+static struct grep_expr *grep_binexp(enum grep_expr_node kind,
+				     struct grep_expr *left,
+				     struct grep_expr *right)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
-	z->node = GREP_NODE_OR;
+	z->node = kind;
 	z->u.binary.left = left;
 	z->u.binary.right = right;
 	return z;
 }
 
+static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)
+{
+	return grep_binexp(GREP_NODE_OR, left, right);
+}
+
 static struct grep_expr *compile_pattern_or(struct grep_pat **);
 static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 {
-- 
2.34.1.455.gd6eb6fd089

[PATCH v2 2/2] grep: use grep_and_expr() in compile_pattern_and()

[Taylor Blau]

In a similar spirit as a previous commit, use the new `grep_and_expr()`
to construct the AND node in `compile_pattern_and()`.

Unlike the aforementioned previous commit, this is not about code
duplication, since this is the only spot in grep.c where an AND node is
constructed. Rather, this is about visual consistency with the other
`compile_pattern_xyz()` functions.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/grep.c b/grep.c
index d772fe6cc5..7a1c52c60a 100644
--- a/grep.c
+++ b/grep.c
@@ -619,6 +619,11 @@ static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *
 	return grep_binexp(GREP_NODE_OR, left, right);
 }
 
+static struct grep_expr *grep_and_expr(struct grep_expr *left, struct grep_expr *right)
+{
+	return grep_binexp(GREP_NODE_AND, left, right);
+}
+
 static struct grep_expr *compile_pattern_or(struct grep_pat **);
 static struct grep_expr *compile_pattern_atom(struct grep_pat **list)
 {
@@ -674,7 +679,7 @@ static struct grep_expr *compile_pattern_not(struct grep_pat **list)
 static struct grep_expr *compile_pattern_and(struct grep_pat **list)
 {
 	struct grep_pat *p;
-	struct grep_expr *x, *y, *z;
+	struct grep_expr *x, *y;
 
 	x = compile_pattern_not(list);
 	p = *list;
@@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)
 		y = compile_pattern_and(list);
 		if (!y)
 			die("--and not followed by pattern expression");
-		CALLOC_ARRAY(z, 1);
-		z->node = GREP_NODE_AND;
-		z->u.binary.left = x;
-		z->u.binary.right = y;
-		return z;
+		return grep_and_expr(x, y);
 	}
 	return x;
 }
-- 
2.34.1.455.gd6eb6fd089

Re: [PATCH 0/2] grep: introduce and use grep_and_expr()

[René Scharfe]

Am 06.01.22 um 20:50 schrieb Taylor Blau:
> René,
>
> Here are a couple of extra patches on top of your series which introduce
> and use a new grep_and_expr() function.
>
> Like the final patch says, this isn't about reducing code duplication,
> but rather about adding visual consistency with the other
> `compile_pattern_xyz()` functions.
>
> Taylor Blau (2):
>   grep: extract grep_binexp() from grep_or_expr()

I considered extracting such a function as well.  I'd have called it
grep_binary_expr(), though, to match the existing names.

I decided against it because it can be misused by passing a non-binary
kind to it.  (That's a weak objection, but the benefit of such a
function was low already in my mind because it doesn't do much.)  You
solve this by keeping grep_or_expr() and adding grep_and_expr(), which
cannot be misused in this way -- OK.

>   grep: use grep_and_expr() in compile_pattern_and()

I think reversing the order of changes would make more sense.  You
wouldn't have to talk about the respective other patch in the commit
messages -- each would stand on their own.  Not worth a reroll, though.

>
>  grep.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> --
> 2.34.1.455.gd6eb6fd089

Re: [PATCH 0/2] grep: introduce and use grep_and_expr()

[Taylor Blau]

On Fri, Jan 07, 2022 at 01:57:17PM +0100, René Scharfe wrote:
> Am 06.01.22 um 20:50 schrieb Taylor Blau:
> > René,
> >
> > Here are a couple of extra patches on top of your series which introduce
> > and use a new grep_and_expr() function.
> >
> > Like the final patch says, this isn't about reducing code duplication,
> > but rather about adding visual consistency with the other
> > `compile_pattern_xyz()` functions.
> >
> > Taylor Blau (2):
> >   grep: extract grep_binexp() from grep_or_expr()
>
> I considered extracting such a function as well.  I'd have called it
> grep_binary_expr(), though, to match the existing names.
>
> I decided against it because it can be misused by passing a non-binary
> kind to it.  (That's a weak objection, but the benefit of such a
> function was low already in my mind because it doesn't do much.)  You
> solve this by keeping grep_or_expr() and adding grep_and_expr(), which
> cannot be misused in this way -- OK.

That makes sense. If it's keeping you up at night, we could easily add a
check to ensure that `kind` is one of GREP_NODE_OR or GREP_NODE_AND. But
I think that any new code that looks like:

    grep_binexp(GREP_NODE_NOT, xyz, NULL);

would probably stick out like a sore thumb. So I doubt that such a check
would buy us much practically speaking.

But I agree that this whole thing probably isn't worth the minimal
effort required, since the couple of patches I posted on top are purely
about cosmetics.

All of that is to say that I'd be happy to see these patches picked up,
and I would also not be sad at all to see them left on the floor.

Thanks,
Taylor

Re: [PATCH 0/2] grep: introduce and use grep_and_expr()

[René Scharfe]

Am 07.01.22 um 20:33 schrieb Taylor Blau:
> On Fri, Jan 07, 2022 at 01:57:17PM +0100, René Scharfe wrote:
>> Am 06.01.22 um 20:50 schrieb Taylor Blau:
>>> René,
>>>
>>> Here are a couple of extra patches on top of your series which introduce
>>> and use a new grep_and_expr() function.
>>>
>>> Like the final patch says, this isn't about reducing code duplication,
>>> but rather about adding visual consistency with the other
>>> `compile_pattern_xyz()` functions.
>>>
>>> Taylor Blau (2):
>>>   grep: extract grep_binexp() from grep_or_expr()
>>
>> I considered extracting such a function as well.  I'd have called it
>> grep_binary_expr(), though, to match the existing names.
>>
>> I decided against it because it can be misused by passing a non-binary
>> kind to it.  (That's a weak objection, but the benefit of such a
>> function was low already in my mind because it doesn't do much.)  You
>> solve this by keeping grep_or_expr() and adding grep_and_expr(), which
>> cannot be misused in this way -- OK.

That "OK" should probably have been a "Good".

>
> That makes sense. If it's keeping you up at night, we could easily add a
> check to ensure that `kind` is one of GREP_NODE_OR or GREP_NODE_AND. But
> I think that any new code that looks like:
>
>     grep_binexp(GREP_NODE_NOT, xyz, NULL);
>
> would probably stick out like a sore thumb. So I doubt that such a check
> would buy us much practically speaking.

Having the wrappers for all the binary node types is good enough.  I'm
not so sure about the sticking out thing -- I managed to miss bigger
mistakes before..

> But I agree that this whole thing probably isn't worth the minimal
> effort required, since the couple of patches I posted on top are purely
> about cosmetics.
>
> All of that is to say that I'd be happy to see these patches picked up,
> and I would also not be sad at all to see them left on the floor.

I think the end result is an improvement.

René