threads / patch / 57198

patch, 2 partsgrep: use grep_or_expr() in compile_pattern_or()

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

## tl;dr

13 messages between Jan 6, 2022 and Jan 7, 2022. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

René Scharfe· Jan 6, 2022, 09:51 UTC · lore

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(-)

Show changes to grep.c +11 −16

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

René Scharfe· Jan 6, 2022, 09:54 UTC · re: René Scharfe · lore

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

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(-)

Show changes to grep.c +11 −14

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

Taylor Blau· Jan 6, 2022, 19:50 UTC · re: René Scharfe · lore

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

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

Taylor Blau· Jan 6, 2022, 19:50 UTC · re: Taylor Blau · lore

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

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(-)

Show changes to grep.c +9 −2

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

Taylor Blau· Jan 6, 2022, 19:50 UTC · re: Taylor Blau · lore

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

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(-)

Show changes to grep.c +6 −5

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

Junio C Hamano· Jan 6, 2022, 22:14 UTC · re: Taylor Blau · lore

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

Taylor Blau <me@ttaylorr.com> writes:

Show 39 quoted lines

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

Taylor Blau· Jan 6, 2022, 22:43 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 06, 2022 at 02:14:47PM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> 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

Taylor Blau· Jan 6, 2022, 22:42 UTC · re: Taylor Blau · lore

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

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

Taylor Blau· Jan 6, 2022, 22:42 UTC · re: Taylor Blau · lore

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

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.

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

Show changes to grep.c +9 −2

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

Taylor Blau· Jan 6, 2022, 22:42 UTC · re: Taylor Blau · lore

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

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

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

Show changes to grep.c +7 −6

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

René Scharfe· Jan 7, 2022, 12:57 UTC · re: Taylor Blau · lore

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

Am 06.01.22 um 20:50 schrieb Taylor Blau:

Show 11 quoted lines

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

Show 6 quoted lines

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

Taylor Blau· Jan 7, 2022, 19:33 UTC · re: René Scharfe · lore

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

On Fri, Jan 07, 2022 at 01:57:17PM +0100, René Scharfe wrote:

Show 21 quoted lines

> 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

René Scharfe· Jan 7, 2022, 19:54 UTC · re: Taylor Blau · lore

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

Am 07.01.22 um 20:33 schrieb Taylor Blau:

Show 22 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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