[v5,16/19] grep.c: refactor free_grep_patterns()

Commit Message

Ævar Arnfjörð Bjarmason Jan. 18, 2023, 12:08 p.m. UTC

Refactor the free_grep_patterns() function to split out the freeing of
the "struct grep_pat" it contains, right now we're only freeing the
"pattern_list", but we should be freeing another member of the same
type, which we'll do in the subsequent commit.

Let's also replace the "return" if we don't have an
"opt->pattern_expression" with a conditional call of
free_pattern_expr().

Before db84376f981 (grep.c: remove "extended" in favor of
"pattern_expression", fix segfault, 2022-10-11) the pattern here was:

	if (!x)
		return;
	free(y);

But after the cleanup in db84376f981 (which was a narrow segfault fix,
and thus avoided refactoring this) we ended up with:

	if (!x)
		return;
	free(x);

Let's instead do:

	if (x)
		free(x);

This doesn't matter for the subsequent change, but as we're
refactoring this function let's make it easier to reason about, and to
extend in the future, i.e. if we start to free free() members that
come after "pattern_expression" in the "struct grep_opt".

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 grep.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Elijah Newren Jan. 26, 2023, 2:26 a.m. UTC | #1

On Wed, Jan 18, 2023 at 5:35 AM Ævar Arnfjörð Bjarmason
<[e-mail]> wrote:
>
> Refactor the free_grep_patterns() function to split out the freeing of
> the "struct grep_pat" it contains, right now we're only freeing the
> "pattern_list", but we should be freeing another member of the same
> type, which we'll do in the subsequent commit.

s/contains, right/contains. Right/

> Let's also replace the "return" if we don't have an
> "opt->pattern_expression" with a conditional call of
> free_pattern_expr().
>
> Before db84376f981 (grep.c: remove "extended" in favor of
> "pattern_expression", fix segfault, 2022-10-11) the pattern here was:
>
>         if (!x)
>                 return;
>         free(y);
>
> But after the cleanup in db84376f981 (which was a narrow segfault fix,
> and thus avoided refactoring this) we ended up with:

For most of your commits, I like the extended history, but in this
case I think it's just a distraction.  I think I'd replace the above
block with just five words:

    While at it, instead of:

>         if (!x)
>                 return;
>         free(x);
>
> Let's instead do:
>
>         if (x)
>                 free(x);

This is slightly confusing, because "if(x) free(x)" can be compressed
to "free(x)".  I think you meant "free_pattern_expr" rather than
"free", which cannot (currently) be similarly compressed.

> This doesn't matter for the subsequent change, but as we're
> refactoring this function let's make it easier to reason about, and to
> extend in the future, i.e. if we start to free free() members that
> come after "pattern_expression" in the "struct grep_opt".

Perhaps just simplify this paragraph to:

This will make it easier to free additional members from
free_grep_patterns() in the future.


>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> ---
>  grep.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 06eed694936..a4450df4559 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -769,11 +769,11 @@ static void free_pattern_expr(struct grep_expr *x)
>         free(x);
>  }
>
> -void free_grep_patterns(struct grep_opt *opt)
> +static void free_grep_pat(struct grep_pat *pattern)
>  {
>         struct grep_pat *p, *n;
>
> -       for (p = opt->pattern_list; p; p = n) {
> +       for (p = pattern; p; p = n) {
>                 n = p->next;
>                 switch (p->token) {
>                 case GREP_PATTERN: /* atom */
> @@ -790,10 +790,14 @@ void free_grep_patterns(struct grep_opt *opt)
>                 }
>                 free(p);
>         }
> +}
>
> -       if (!opt->pattern_expression)
> -               return;
> -       free_pattern_expr(opt->pattern_expression);
> +void free_grep_patterns(struct grep_opt *opt)
> +{
> +       free_grep_pat(opt->pattern_list);
> +
> +       if (opt->pattern_expression)
> +               free_pattern_expr(opt->pattern_expression);
>  }

I think this last function would read even better as:

+void free_grep_patterns(struct grep_opt *opt)
+{
+       free_grep_pat(opt->pattern_list);
+       free_pattern_expr(opt->pattern_expression);
 }

after modifying free_pattern_expr() to return early if passed a NULL argument.

Patch

diff --git a/grep.c b/grep.c
index 06eed694936..a4450df4559 100644
--- a/grep.c
+++ b/grep.c
@@ -769,11 +769,11 @@  static void free_pattern_expr(struct grep_expr *x)
 	free(x);
 }
 
-void free_grep_patterns(struct grep_opt *opt)
+static void free_grep_pat(struct grep_pat *pattern)
 {
 	struct grep_pat *p, *n;
 
-	for (p = opt->pattern_list; p; p = n) {
+	for (p = pattern; p; p = n) {
 		n = p->next;
 		switch (p->token) {
 		case GREP_PATTERN: /* atom */
@@ -790,10 +790,14 @@  void free_grep_patterns(struct grep_opt *opt)
 		}
 		free(p);
 	}
+}
 
-	if (!opt->pattern_expression)
-		return;
-	free_pattern_expr(opt->pattern_expression);
+void free_grep_patterns(struct grep_opt *opt)
+{
+	free_grep_pat(opt->pattern_list);
+
+	if (opt->pattern_expression)
+		free_pattern_expr(opt->pattern_expression);
 }
 
 static const char *end_of_line(const char *cp, unsigned long *left)