Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Thu, May 5, 2022 at 3:47 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/unit/test-qga.c | 125 +++++++++++++++---------------------------
>> >  1 file changed, 45 insertions(+), 80 deletions(-)
>> >
>> > diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
>> > index ab0b12a2dd16..53cefc2c2649 100644
>> > --- a/tests/unit/test-qga.c
>> > +++ b/tests/unit/test-qga.c
>> > @@ -51,8 +51,11 @@ static void
>> >  fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
>> >  {
>> >      const gchar *extra_arg = data;
>> > -    GError *error = NULL;
>> > -    gchar *cwd, *path, *cmd, **argv = NULL;
>> > +    g_autoptr(GError) error = NULL;
>> > +    g_autofree char *cwd = NULL;
>> > +    g_autofree char *path = NULL;
>> > +    g_autofree char *cmd = NULL;
>> > +    g_auto(GStrv) argv = NULL;
>>
>> Arranges five variables to be auto-freed, where ...
>>
>> >
>> >      fixture->loop = g_main_loop_new(NULL, FALSE);
>> >
>> > @@ -78,17 +81,12 @@ fixture_setup(TestFixture *fixture, gconstpointer 
>> > data, gchar **envp)
>> >
>> >      fixture->fd = connect_qga(path);
>> >      g_assert_cmpint(fixture->fd, !=, -1);
>> > -
>> > -    g_strfreev(argv);
>> > -    g_free(cmd);
>> > -    g_free(cwd);
>> > -    g_free(path);
>>
>> ... only four were freed before.  The extra one is @error.  Which can
>> only be null here, thanks to g_assert_no_error(), can't it?
>
> Right, but for consistency we can have it. I can drop it too.

If you want to add no-op frees for consistency, then your commit message
should prepare reviewers for that.

And you should perhaps be prepared for reviewers asking you to split
your patch ;)

Dropping seems the easiest path forward.