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

From: Elijah Newren <newren@gmail.com>
Date: Jan 26, 2023, 02:26 UTC
Message-ID: <CABPp-BGucUNYP8TK_aDs4AEPWg2NwaqDXbnqzNX9OZ5aFc4daQ@mail.gmail.com>
In-Reply-To: <patch-v5-16.19-10959760dfc-20230118T120334Z-avarab@gmail.com>

On Wed, Jan 18, 2023 at 5:35 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 5 quoted lines

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

Show 13 quoted lines

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

Show 8 quoted lines

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

Show 40 quoted lines

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  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.