{"thread":{"id":"57198","subject":"[PATCH 1/2] grep: use grep_or_expr() in compile_pattern_or()","startedAt":"2022-01-06T09:51:07Z","lastAt":"2022-01-07T19:54:28Z","messageCount":13,"participants":["René Scharfe","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"445618","messageId":"ea01f9b6-2ce3-57e8-533f-4cc29937f700@web.de","threadId":"57198","inReplyTo":null,"subject":"[PATCH 1/2] grep: use grep_or_expr() in compile_pattern_or()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-01-06T09:51:00Z","receivedAt":"2022-01-06T09:51:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the definition of grep_or_expr() up and use this function in\ncompile_pattern_or() to reduce code duplication.\n\nSigned-off-by: René Scharfe \n---\n grep.c | 26 +++++++++++---------------\n 1 file changed, 11 insertions(+), 15 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 47c75ab7fb..f1bbe80ccb 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -595,6 +595,15 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t}\n }\n\n+static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n+{\n+\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n+\tz->node = GREP_NODE_OR;\n+\tz->u.binary.left = left;\n+\tz->u.binary.right = right;\n+\treturn z;\n+}\n+\n static struct grep_expr *compile_pattern_or(struct grep_pat **);\n static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n {\n@@ -677,7 +686,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n static struct grep_expr *compile_pattern_or(struct grep_pat **list)\n {\n \tstruct grep_pat *p;\n-\tstruct grep_expr *x, *y, *z;\n+\tstruct grep_expr *x, *y;\n\n \tx = compile_pattern_and(list);\n \tp = *list;\n@@ -685,11 +694,7 @@ static struct grep_expr *compile_pattern_or(struct grep_pat **list)\n \t\ty = compile_pattern_or(list);\n \t\tif (!y)\n \t\t\tdie(\"not a pattern expression %s\", p->pattern);\n-\t\tCALLOC_ARRAY(z, 1);\n-\t\tz->node = GREP_NODE_OR;\n-\t\tz->u.binary.left = x;\n-\t\tz->u.binary.right = y;\n-\t\treturn z;\n+\t\treturn grep_or_expr(x, y);\n \t}\n \treturn x;\n }\n@@ -714,15 +719,6 @@ static struct grep_expr *grep_true_expr(void)\n \treturn z;\n }\n\n-static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n-{\n-\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n-\tz->node = GREP_NODE_OR;\n-\tz->u.binary.left = left;\n-\tz->u.binary.right = right;\n-\treturn z;\n-}\n-\n static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n {\n \tstruct grep_pat *p;\n--\n2.34.1\n"},{"id":"445619","messageId":"a913ad56-9083-d124-f1ec-58a1f48d48fc@web.de","threadId":"57198","inReplyTo":"ea01f9b6-2ce3-57e8-533f-4cc29937f700@web.de","subject":"[PATCH 2/2] grep: use grep_not_expr() in compile_pattern_not()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-01-06T09:54:19Z","receivedAt":"2022-01-06T09:54:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the definition of grep_not_expr() up and use this function in\ncompile_pattern_not() to simplify the code and reduce duplication.\n\nSigned-off-by: René Scharfe \n---\n grep.c | 24 +++++++++++-------------\n 1 file changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex f1bbe80ccb..bdbd06d437 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -595,6 +595,14 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \t}\n }\n\n+static struct grep_expr *grep_not_expr(struct grep_expr *expr)\n+{\n+\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n+\tz->node = GREP_NODE_NOT;\n+\tz->u.unary = expr;\n+\treturn z;\n+}\n+\n static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n {\n \tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n@@ -647,12 +655,10 @@ static struct grep_expr *compile_pattern_not(struct grep_pat **list)\n \t\tif (!p->next)\n \t\t\tdie(\"--not not followed by pattern expression\");\n \t\t*list = p->next;\n-\t\tCALLOC_ARRAY(x, 1);\n-\t\tx->node = GREP_NODE_NOT;\n-\t\tx->u.unary = compile_pattern_not(list);\n-\t\tif (!x->u.unary)\n+\t\tx = compile_pattern_not(list);\n+\t\tif (!x)\n \t\t\tdie(\"--not followed by non pattern expression\");\n-\t\treturn x;\n+\t\treturn grep_not_expr(x);\n \tdefault:\n \t\treturn compile_pattern_atom(list);\n \t}\n@@ -704,14 +710,6 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)\n \treturn compile_pattern_or(list);\n }\n\n-static struct grep_expr *grep_not_expr(struct grep_expr *expr)\n-{\n-\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n-\tz->node = GREP_NODE_NOT;\n-\tz->u.unary = expr;\n-\treturn z;\n-}\n-\n static struct grep_expr *grep_true_expr(void)\n {\n \tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n--\n2.34.1\n"},{"id":"445647","messageId":"cover.1641498525.git.me@ttaylorr.com","threadId":"57198","inReplyTo":"ea01f9b6-2ce3-57e8-533f-4cc29937f700@web.de","subject":"[PATCH 0/2] grep: introduce and use grep_and_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T19:50:09Z","receivedAt":"2022-01-06T19:50:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"René,\n\nHere are a couple of extra patches on top of your series which introduce\nand use a new grep_and_expr() function.\n\nLike the final patch says, this isn't about reducing code duplication,\nbut rather about adding visual consistency with the other\n`compile_pattern_xyz()` functions.\n\nTaylor Blau (2):\n grep: extract grep_binexp() from grep_or_expr()\n grep: use grep_and_expr() in compile_pattern_and()\n\n grep.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\n--\n2.34.1.455.gd6eb6fd089\n"},{"id":"445648","messageId":"dae476d1bd14608358b6c103e2b5dda76d294687.1641498525.git.me@ttaylorr.com","threadId":"57198","inReplyTo":"cover.1641498525.git.me@ttaylorr.com","subject":"[PATCH 1/2] grep: extract grep_binexp() from grep_or_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T19:50:12Z","receivedAt":"2022-01-06T19:50:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When constructing an OR node, the grep.c code uses `grep_or_expr()` to\nmake a node, assign its kind, and set its left and right children. The\nsame is not done for AND nodes.\n\nPrepare to introduce a new `grep_and_expr()` function which will share\ncode with the existing implementation of `grep_or_expr()` by introducing\na new function which compiles either kind of binary expression, and\nreimplement `grep_or_expr()` in terms of it.\n\nSigned-off-by: Taylor Blau \n---\n grep.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex bdbd06d437..d772fe6cc5 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -603,15 +603,22 @@ static struct grep_expr *grep_not_expr(struct grep_expr *expr)\n \treturn z;\n }\n \n-static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n+static struct grep_expr *grep_binexp(enum grep_expr_node kind,\n+\t\t\t\t struct grep_expr *left,\n+\t\t\t\t struct grep_expr *right)\n {\n \tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n-\tz->node = GREP_NODE_OR;\n+\tz->node = kind;\n \tz->u.binary.left = left;\n \tz->u.binary.right = right;\n \treturn z;\n }\n \n+static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n+{\n+\treturn grep_binexp(GREP_NODE_OR, left, right);\n+}\n+\n static struct grep_expr *compile_pattern_or(struct grep_pat **);\n static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n {\n-- \n2.34.1.455.gd6eb6fd089\n\n"},{"id":"445649","messageId":"71bd2d1bcc6486f2e21c649708d3f4fa67dc162c.1641498525.git.me@ttaylorr.com","threadId":"57198","inReplyTo":"cover.1641498525.git.me@ttaylorr.com","subject":"[PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T19:50:15Z","receivedAt":"2022-01-06T19:50:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as a previous commit, use the new `grep_and_expr()`\nto construct the AND node in `compile_pattern_and()`.\n\nUnlike the aforementioned previous commit, this is not about code\nduplication, since this is the only spot in grep.c where an AND node is\nconstructed. Rather, this is about visual consistency with the other\n`compile_pattern_xyz()` functions.\n\nSigned-off-by: Taylor Blau \n---\n grep.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex d772fe6cc5..ab4fdacaed 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -619,6 +619,11 @@ static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *\n \treturn grep_binexp(GREP_NODE_OR, left, right);\n }\n \n+static struct grep_expr *grep_and_expr(struct grep_expr *left, struct grep_expr *right)\n+{\n+\treturn grep_binexp(GREP_NODE_AND, left, right);\n+}\n+\n static struct grep_expr *compile_pattern_or(struct grep_pat **);\n static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n {\n@@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n \t\ty = compile_pattern_and(list);\n \t\tif (!y)\n \t\t\tdie(\"--and not followed by pattern expression\");\n-\t\tCALLOC_ARRAY(z, 1);\n-\t\tz->node = GREP_NODE_AND;\n-\t\tz->u.binary.left = x;\n-\t\tz->u.binary.right = y;\n-\t\treturn z;\n+\t\treturn grep_and_expr(x, y);\n \t}\n \treturn x;\n }\n-- \n2.34.1.455.gd6eb6fd089\n"},{"id":"445662","messageId":"xmqqlezsbjfs.fsf@gitster.g","threadId":"57198","inReplyTo":"71bd2d1bcc6486f2e21c649708d3f4fa67dc162c.1641498525.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-06T22:14:47Z","receivedAt":"2022-01-06T22:14:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> In a similar spirit as a previous commit, use the new `grep_and_expr()`\n> to construct the AND node in `compile_pattern_and()`.\n>\n> Unlike the aforementioned previous commit, this is not about code\n> duplication, since this is the only spot in grep.c where an AND node is\n> constructed. Rather, this is about visual consistency with the other\n> `compile_pattern_xyz()` functions.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> grep.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index d772fe6cc5..ab4fdacaed 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -619,6 +619,11 @@ static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *\n> \treturn grep_binexp(GREP_NODE_OR, left, right);\n> }\n> \n> +static struct grep_expr *grep_and_expr(struct grep_expr *left, struct grep_expr *right)\n> +{\n> +\treturn grep_binexp(GREP_NODE_AND, left, right);\n> +}\n> +\n> static struct grep_expr *compile_pattern_or(struct grep_pat **);\n> static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n> {\n> @@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n> \t\ty = compile_pattern_and(list);\n> \t\tif (!y)\n> \t\t\tdie(\"--and not followed by pattern expression\");\n> -\t\tCALLOC_ARRAY(z, 1);\n> -\t\tz->node = GREP_NODE_AND;\n> -\t\tz->u.binary.left = x;\n> -\t\tz->u.binary.right = y;\n> -\t\treturn z;\n> +\t\treturn grep_and_expr(x, y);\n\nYou'd need to remove 'z' from the function to avoid getting yelled\nat by your compiler for unused variable.\n\n"},{"id":"445669","messageId":"cover.1641508884.git.me@ttaylorr.com","threadId":"57198","inReplyTo":"cover.1641498525.git.me@ttaylorr.com","subject":"[PATCH v2 0/2] grep: introduce and use grep_and_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T22:42:17Z","receivedAt":"2022-01-06T22:42:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This small reroll fixes an issue in the second patch where a stack\nvariable was left unused.\n\nTaylor Blau (2):\n grep: extract grep_binexp() from grep_or_expr()\n grep: use grep_and_expr() in compile_pattern_and()\n\n grep.c | 24 ++++++++++++++++--------\n 1 file changed, 16 insertions(+), 8 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: dae476d1bd grep: extract grep_binexp() from grep_or_expr()\n1: 71bd2d1bcc ! 2: f637e02422 grep: use grep_and_expr() in compile_pattern_and()\n @@ grep.c: static struct grep_expr *grep_or_expr(struct grep_expr *left, struct gre\n static struct grep_expr *compile_pattern_or(struct grep_pat **);\n static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n {\n +@@ grep.c: static struct grep_expr *compile_pattern_not(struct grep_pat **list)\n + static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n + {\n + \tstruct grep_pat *p;\n +-\tstruct grep_expr *x, *y, *z;\n ++\tstruct grep_expr *x, *y;\n +\n + \tx = compile_pattern_not(list);\n + \tp = *list;\n @@ grep.c: static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n \t\ty = compile_pattern_and(list);\n \t\tif (!y)\n--\n2.34.1.455.gd6eb6fd089\n"},{"id":"445670","messageId":"dae476d1bd14608358b6c103e2b5dda76d294687.1641508884.git.me@ttaylorr.com","threadId":"57198","inReplyTo":"cover.1641508884.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] grep: extract grep_binexp() from grep_or_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T22:42:20Z","receivedAt":"2022-01-06T22:42:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When constructing an OR node, the grep.c code uses `grep_or_expr()` to\nmake a node, assign its kind, and set its left and right children. The\nsame is not done for AND nodes.\n\nPrepare to introduce a new `grep_and_expr()` function which will share\ncode with the existing implementation of `grep_or_expr()` by introducing\na new function which compiles either kind of binary expression, and\nreimplement `grep_or_expr()` in terms of it.\n\nSigned-off-by: Taylor Blau \n---\n grep.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex bdbd06d437..d772fe6cc5 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -603,15 +603,22 @@ static struct grep_expr *grep_not_expr(struct grep_expr *expr)\n \treturn z;\n }\n \n-static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n+static struct grep_expr *grep_binexp(enum grep_expr_node kind,\n+\t\t\t\t struct grep_expr *left,\n+\t\t\t\t struct grep_expr *right)\n {\n \tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n-\tz->node = GREP_NODE_OR;\n+\tz->node = kind;\n \tz->u.binary.left = left;\n \tz->u.binary.right = right;\n \treturn z;\n }\n \n+static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n+{\n+\treturn grep_binexp(GREP_NODE_OR, left, right);\n+}\n+\n static struct grep_expr *compile_pattern_or(struct grep_pat **);\n static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n {\n-- \n2.34.1.455.gd6eb6fd089\n\n"},{"id":"445671","messageId":"f637e024222a98b6a064fd49b6b810c0b09527d8.1641508884.git.me@ttaylorr.com","threadId":"57198","inReplyTo":"cover.1641508884.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] grep: use grep_and_expr() in compile_pattern_and()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T22:42:28Z","receivedAt":"2022-01-06T22:42:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as a previous commit, use the new `grep_and_expr()`\nto construct the AND node in `compile_pattern_and()`.\n\nUnlike the aforementioned previous commit, this is not about code\nduplication, since this is the only spot in grep.c where an AND node is\nconstructed. Rather, this is about visual consistency with the other\n`compile_pattern_xyz()` functions.\n\nSigned-off-by: Taylor Blau \n---\n grep.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex d772fe6cc5..7a1c52c60a 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -619,6 +619,11 @@ static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *\n \treturn grep_binexp(GREP_NODE_OR, left, right);\n }\n \n+static struct grep_expr *grep_and_expr(struct grep_expr *left, struct grep_expr *right)\n+{\n+\treturn grep_binexp(GREP_NODE_AND, left, right);\n+}\n+\n static struct grep_expr *compile_pattern_or(struct grep_pat **);\n static struct grep_expr *compile_pattern_atom(struct grep_pat **list)\n {\n@@ -674,7 +679,7 @@ static struct grep_expr *compile_pattern_not(struct grep_pat **list)\n static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n {\n \tstruct grep_pat *p;\n-\tstruct grep_expr *x, *y, *z;\n+\tstruct grep_expr *x, *y;\n \n \tx = compile_pattern_not(list);\n \tp = *list;\n@@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n \t\ty = compile_pattern_and(list);\n \t\tif (!y)\n \t\t\tdie(\"--and not followed by pattern expression\");\n-\t\tCALLOC_ARRAY(z, 1);\n-\t\tz->node = GREP_NODE_AND;\n-\t\tz->u.binary.left = x;\n-\t\tz->u.binary.right = y;\n-\t\treturn z;\n+\t\treturn grep_and_expr(x, y);\n \t}\n \treturn x;\n }\n-- \n2.34.1.455.gd6eb6fd089\n"},{"id":"445672","messageId":"YddwijcMrmkgJNmD@nand.local","threadId":"57198","inReplyTo":"xmqqlezsbjfs.fsf@gitster.g","subject":"Re: [PATCH 2/2] grep: use grep_and_expr() in compile_pattern_and()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T22:43:22Z","receivedAt":"2022-01-06T22:43:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jan 06, 2022 at 02:14:47PM -0800, Junio C Hamano wrote:\n> Taylor Blau writes:\n> > @@ -687,11 +692,7 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n> > \t\ty = compile_pattern_and(list);\n> > \t\tif (!y)\n> > \t\t\tdie(\"--and not followed by pattern expression\");\n> > -\t\tCALLOC_ARRAY(z, 1);\n> > -\t\tz->node = GREP_NODE_AND;\n> > -\t\tz->u.binary.left = x;\n> > -\t\tz->u.binary.right = y;\n> > -\t\treturn z;\n> > +\t\treturn grep_and_expr(x, y);\n>\n> You'd need to remove 'z' from the function to avoid getting yelled\n> at by your compiler for unused variable.\n\nHow embarrassing :-). Thanks for noticing, I wrote this so hastily I\nneglected to even compile it with DEVELOPER=1.\n\nThe new version I sent in response fixes this issue.\n\nThanks,\nTaylor\n"},{"id":"445713","messageId":"c573cc00-d5b3-9af1-a627-6c2462cae3be@web.de","threadId":"57198","inReplyTo":"cover.1641498525.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] grep: introduce and use grep_and_expr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-01-07T12:57:17Z","receivedAt":"2022-01-07T12:57:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.01.22 um 20:50 schrieb Taylor Blau:\n> René,\n>\n> Here are a couple of extra patches on top of your series which introduce\n> and use a new grep_and_expr() function.\n>\n> Like the final patch says, this isn't about reducing code duplication,\n> but rather about adding visual consistency with the other\n> `compile_pattern_xyz()` functions.\n>\n> Taylor Blau (2):\n> grep: extract grep_binexp() from grep_or_expr()\n\nI considered extracting such a function as well. I'd have called it\ngrep_binary_expr(), though, to match the existing names.\n\nI decided against it because it can be misused by passing a non-binary\nkind to it. (That's a weak objection, but the benefit of such a\nfunction was low already in my mind because it doesn't do much.) You\nsolve this by keeping grep_or_expr() and adding grep_and_expr(), which\ncannot be misused in this way -- OK.\n\n> grep: use grep_and_expr() in compile_pattern_and()\n\nI think reversing the order of changes would make more sense. You\nwouldn't have to talk about the respective other patch in the commit\nmessages -- each would stand on their own. Not worth a reroll, though.\n\n>\n> grep.c | 22 +++++++++++++++-------\n> 1 file changed, 15 insertions(+), 7 deletions(-)\n>\n> --\n> 2.34.1.455.gd6eb6fd089\n\n"},{"id":"445737","messageId":"YdiVmgJYPzVrIYE1@nand.local","threadId":"57198","inReplyTo":"c573cc00-d5b3-9af1-a627-6c2462cae3be@web.de","subject":"Re: [PATCH 0/2] grep: introduce and use grep_and_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-07T19:33:46Z","receivedAt":"2022-01-07T19:33:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 07, 2022 at 01:57:17PM +0100, René Scharfe wrote:\n> Am 06.01.22 um 20:50 schrieb Taylor Blau:\n> > René,\n> >\n> > Here are a couple of extra patches on top of your series which introduce\n> > and use a new grep_and_expr() function.\n> >\n> > Like the final patch says, this isn't about reducing code duplication,\n> > but rather about adding visual consistency with the other\n> > `compile_pattern_xyz()` functions.\n> >\n> > Taylor Blau (2):\n> > grep: extract grep_binexp() from grep_or_expr()\n>\n> I considered extracting such a function as well. I'd have called it\n> grep_binary_expr(), though, to match the existing names.\n>\n> I decided against it because it can be misused by passing a non-binary\n> kind to it. (That's a weak objection, but the benefit of such a\n> function was low already in my mind because it doesn't do much.) You\n> solve this by keeping grep_or_expr() and adding grep_and_expr(), which\n> cannot be misused in this way -- OK.\n\nThat makes sense. If it's keeping you up at night, we could easily add a\ncheck to ensure that `kind` is one of GREP_NODE_OR or GREP_NODE_AND. But\nI think that any new code that looks like:\n\n grep_binexp(GREP_NODE_NOT, xyz, NULL);\n\nwould probably stick out like a sore thumb. So I doubt that such a check\nwould buy us much practically speaking.\n\nBut I agree that this whole thing probably isn't worth the minimal\neffort required, since the couple of patches I posted on top are purely\nabout cosmetics.\n\nAll of that is to say that I'd be happy to see these patches picked up,\nand I would also not be sad at all to see them left on the floor.\n\nThanks,\nTaylor\n"},{"id":"445742","messageId":"ec92e2ec-ef0a-8fe3-abcb-5cc6b29de762@web.de","threadId":"57198","inReplyTo":"YdiVmgJYPzVrIYE1@nand.local","subject":"Re: [PATCH 0/2] grep: introduce and use grep_and_expr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-01-07T19:54:16Z","receivedAt":"2022-01-07T19:54:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.01.22 um 20:33 schrieb Taylor Blau:\n> On Fri, Jan 07, 2022 at 01:57:17PM +0100, René Scharfe wrote:\n>> Am 06.01.22 um 20:50 schrieb Taylor Blau:\n>>> René,\n>>>\n>>> Here are a couple of extra patches on top of your series which introduce\n>>> and use a new grep_and_expr() function.\n>>>\n>>> Like the final patch says, this isn't about reducing code duplication,\n>>> but rather about adding visual consistency with the other\n>>> `compile_pattern_xyz()` functions.\n>>>\n>>> Taylor Blau (2):\n>>> grep: extract grep_binexp() from grep_or_expr()\n>>\n>> I considered extracting such a function as well. I'd have called it\n>> grep_binary_expr(), though, to match the existing names.\n>>\n>> I decided against it because it can be misused by passing a non-binary\n>> kind to it. (That's a weak objection, but the benefit of such a\n>> function was low already in my mind because it doesn't do much.) You\n>> solve this by keeping grep_or_expr() and adding grep_and_expr(), which\n>> cannot be misused in this way -- OK.\n\nThat \"OK\" should probably have been a \"Good\".\n\n>\n> That makes sense. If it's keeping you up at night, we could easily add a\n> check to ensure that `kind` is one of GREP_NODE_OR or GREP_NODE_AND. But\n> I think that any new code that looks like:\n>\n> grep_binexp(GREP_NODE_NOT, xyz, NULL);\n>\n> would probably stick out like a sore thumb. So I doubt that such a check\n> would buy us much practically speaking.\n\nHaving the wrappers for all the binary node types is good enough. I'm\nnot so sure about the sticking out thing -- I managed to miss bigger\nmistakes before..\n\n> But I agree that this whole thing probably isn't worth the minimal\n> effort required, since the couple of patches I posted on top are purely\n> about cosmetics.\n>\n> All of that is to say that I'd be happy to see these patches picked up,\n> and I would also not be sad at all to see them left on the floor.\n\nI think the end result is an improvement.\n\nRené\n"}]}