On Wed, Jan 18, 2023 at 5:35 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> 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 <avarab@xxxxxxxxx>
> ---
> 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.
