# [PATCH 1/2] grep: use grep_or_expr() in compile_pattern_or() 13 messages from 2022-01-06 to 2022-01-07. Participants: René Scharfe, Taylor Blau, Junio C Hamano. Thread: https://gitlist.dev/t/57198 ## René Scharfe, 2022-01-06 09:51 Subject: [PATCH 1/2] grep: use grep_or_expr() in compile_pattern_or() Message-ID: URL: https://gitlist.dev/e/ea01f9b6-2ce3-57e8-533f-4cc29937f700%40web.de ``` 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 --- 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 ``` ## René Scharfe, 2022-01-06 09:54 Subject: [PATCH 2/2] grep: use grep_not_expr() in compile_pattern_not() Message-ID: URL: https://gitlist.dev/e/a913ad56-9083-d124-f1ec-58a1f48d48fc%40web.de In-Reply-To: ``` 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 --- 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 ``` ## Taylor Blau, 2022-01-06 19:50 Subject: [PATCH 0/2] grep: introduce and use grep_and_expr() Message-ID: URL: https://gitlist.dev/e/cover.1641498525.git.me%40ttaylorr.com In-Reply-To: ``` 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, 2022-01-06 19:50 Subject: [PATCH 1/2] grep: extract grep_binexp() from grep_or_expr() Message-ID: URL: https://gitlist.dev/e/dae476d1bd14608358b6c103e2b5dda76d294687.1641498525.git.me%40ttaylorr.com In-Reply-To: ``` 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 --- 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 ``` ## Taylor Blau, 2022-01-06 19:50 Subject: [PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and() Message-ID: <71bd2d1bcc6486f2e21c649708d3f4fa67dc162c.1641498525.git.me@ttaylorr.com> URL: https://gitlist.dev/e/71bd2d1bcc6486f2e21c649708d3f4fa67dc162c.1641498525.git.me%40ttaylorr.com In-Reply-To: ``` 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 --- 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 ``` ## Junio C Hamano, 2022-01-06 22:14 Subject: Re: [PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and() Message-ID: URL: https://gitlist.dev/e/xmqqlezsbjfs.fsf%40gitster.g In-Reply-To: <71bd2d1bcc6486f2e21c649708d3f4fa67dc162c.1641498525.git.me@ttaylorr.com> ``` Taylor Blau 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 > --- > 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, 2022-01-06 22:42 Subject: [PATCH v2 0/2] grep: introduce and use grep_and_expr() Message-ID: URL: https://gitlist.dev/e/cover.1641508884.git.me%40ttaylorr.com In-Reply-To: ``` 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, 2022-01-06 22:42 Subject: [PATCH v2 1/2] grep: extract grep_binexp() from grep_or_expr() Message-ID: URL: https://gitlist.dev/e/dae476d1bd14608358b6c103e2b5dda76d294687.1641508884.git.me%40ttaylorr.com In-Reply-To: ``` 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 --- 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 ``` ## Taylor Blau, 2022-01-06 22:42 Subject: [PATCH v2 2/2] grep: use grep_and_expr() in compile_pattern_and() Message-ID: URL: https://gitlist.dev/e/f637e024222a98b6a064fd49b6b810c0b09527d8.1641508884.git.me%40ttaylorr.com In-Reply-To: ``` 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 --- 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 ``` ## Taylor Blau, 2022-01-06 22:43 Subject: Re: [PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and() Message-ID: URL: https://gitlist.dev/e/YddwijcMrmkgJNmD%40nand.local In-Reply-To: ``` On Thu, Jan 06, 2022 at 02:14:47PM -0800, Junio C Hamano wrote: > Taylor Blau 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 ``` ## René Scharfe, 2022-01-07 12:57 Subject: Re: [PATCH 0/2] grep: introduce and use grep_and_expr() Message-ID: URL: https://gitlist.dev/e/c573cc00-d5b3-9af1-a627-6c2462cae3be%40web.de In-Reply-To: ``` 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 ``` ## Taylor Blau, 2022-01-07 19:33 Subject: Re: [PATCH 0/2] grep: introduce and use grep_and_expr() Message-ID: URL: https://gitlist.dev/e/YdiVmgJYPzVrIYE1%40nand.local In-Reply-To: ``` 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 ``` ## René Scharfe, 2022-01-07 19:54 Subject: Re: [PATCH 0/2] grep: introduce and use grep_and_expr() Message-ID: URL: https://gitlist.dev/e/ec92e2ec-ef0a-8fe3-abcb-5cc6b29de762%40web.de In-Reply-To: ``` 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é ```