qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Drop superfluous conditionals ar


From: Markus Armbruster
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] Drop superfluous conditionals around g_strdup()
Date: Thu, 04 Dec 2014 13:43:42 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Fam Zheng <address@hidden> writes:

> On Thu, 12/04 11:39, Markus Armbruster wrote:
>> Fam Zheng <address@hidden> writes:
>> 
>> > On Thu, 12/04 10:26, Markus Armbruster wrote:
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >>  backends/rng-random.c    |  6 +-----
>> >>  hw/tpm/tpm_passthrough.c |  4 +---
>> >>  util/uri.c               | 43 +++++++++++++++++--------------------------
>> >>  3 files changed, 19 insertions(+), 34 deletions(-)
>> >> 
>> >> diff --git a/backends/rng-random.c b/backends/rng-random.c
>> >> index 601d9dc..4f85a8e 100644
>> >> --- a/backends/rng-random.c
>> >> +++ b/backends/rng-random.c
>> >> @@ -88,11 +88,7 @@ static char *rng_random_get_filename(Object *obj, 
>> >> Error **errp)
>> >>  {
>> >>      RndRandom *s = RNG_RANDOM(obj);
>> >>  
>> >> -    if (s->filename) {
>> >> -        return g_strdup(s->filename);
>> >> -    }
>> >> -
>> >> -    return NULL;
>> >> +    return g_strdup(s->filename);
>> >>  }
>> >>  
>> >>  static void rng_random_set_filename(Object *obj, const char *filename,
>> >> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
>> >> index 56e9e0f..2bf3c6f 100644
>> >> --- a/hw/tpm/tpm_passthrough.c
>> >> +++ b/hw/tpm/tpm_passthrough.c
>> >> @@ -400,9 +400,7 @@ static int 
>> >> tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)
>> >>      const char *value;
>> >>  
>> >>      value = qemu_opt_get(opts, "cancel-path");
>> >> -    if (value) {
>> >> -        tb->cancel_path = g_strdup(value);
>> >> -    }
>> >> +    tb->cancel_path = g_strdup(value);
>> >>  
>> >>      value = qemu_opt_get(opts, "path");
>> >>      if (!value) {
>> >> diff --git a/util/uri.c b/util/uri.c
>> >> index e348c17..bbf2832 100644
>> >> --- a/util/uri.c
>> >> +++ b/util/uri.c
>> >> @@ -1736,24 +1736,21 @@ uri_resolve(const char *uri, const char *base) {
>> >>   goto done;
>> >>      if ((ref->scheme == NULL) && (ref->path == NULL) &&
>> >>   ((ref->authority == NULL) && (ref->server == NULL))) {
>> >> - if (bas->scheme != NULL)
>> >> -     res->scheme = g_strdup(bas->scheme);
>> >> +        res->scheme = g_strdup(bas->scheme);
>> >>   if (bas->authority != NULL)
>> >>       res->authority = g_strdup(bas->authority);
>> >>   else if (bas->server != NULL) {
>> >> -     res->server = g_strdup(bas->server);
>> >> -     if (bas->user != NULL)
>> >> -         res->user = g_strdup(bas->user);
>> >> -     res->port = bas->port;
>> >> +            res->server = g_strdup(bas->server);
>> >> +            res->user = g_strdup(bas->user);
>> >> +            res->port = bas->port;
>
> You fixed indentation for res->server and res->port ...

... because I touched these lines...

>> >>   }
>> >> - if (bas->path != NULL)
>> >> -     res->path = g_strdup(bas->path);
>> >> - if (ref->query != NULL)
>> >> +        res->path = g_strdup(bas->path);
>> >> +        if (ref->query != NULL) {
>> >>       res->query = g_strdup (ref->query);
>
> ... but not for res->query.

... because I didn't touch this line.

>> >> +        } else {
>> >> +            res->query = g_strdup(bas->query);
>> >> +        }
>> >> +        res->fragment = g_strdup(ref->fragment);
>> >>   goto step_7;
>> >>      }
>> >>  
>> >> @@ -1767,13 +1764,10 @@ uri_resolve(const char *uri, const char *base) {
>> >>   val = uri_to_string(ref);
>> >>   goto done;
>> >>      }
>> >> -    if (bas->scheme != NULL)
>> >> - res->scheme = g_strdup(bas->scheme);
>> >> +    res->scheme = g_strdup(bas->scheme);
>> >>  
>> >> -    if (ref->query != NULL)
>> >> - res->query = g_strdup(ref->query);
>> >> -    if (ref->fragment != NULL)
>> >> - res->fragment = g_strdup(ref->fragment);
>> >> +    res->query = g_strdup(ref->query);
>> >> +    res->fragment = g_strdup(ref->fragment);
>> >>  
>> >>      /*
>> >>       * 4) If the authority component is defined, then the reference is a
>> >> @@ -1787,20 +1781,17 @@ uri_resolve(const char *uri, const char *base) {
>> >>       res->authority = g_strdup(ref->authority);
>> >>   else {
>> >>       res->server = g_strdup(ref->server);
>> >> -     if (ref->user != NULL)
>> >> -         res->user = g_strdup(ref->user);
>> >> +            res->user = g_strdup(ref->user);
>> >>              res->port = ref->port;
>> >>   }
>> >> - if (ref->path != NULL)
>> >> -     res->path = g_strdup(ref->path);
>> >> +        res->path = g_strdup(ref->path);
>> >>   goto step_7;
>> >>      }
>> >>      if (bas->authority != NULL)
>> >>   res->authority = g_strdup(bas->authority);
>> >>      else if (bas->server != NULL) {
>> >> - res->server = g_strdup(bas->server);
>> >> - if (bas->user != NULL)
>> >> -     res->user = g_strdup(bas->user);
>> >> +        res->server = g_strdup(bas->server);
>> >> +        res->user = g_strdup(bas->user);
>> >>   res->port = bas->port;
>
> and not for res->port.

Likewise.

>> >>      }
>> >>  
>> >> -- 
>> >> 1.9.3
>> >> 
>> >> 
>> >
>> > Very confusing tab/whitespace mixture. Code is better, format is
>> > worse. I'm not
>> > sure it's a win. :)
>> 
>> As per standard operating procedure, I expanded tabs in the lines I
>> touched.  No visual difference, except in patches.
>> 
>> What do you want me to do?
>> 
>> 1. Don't expand tabs, ignore checkpatch.pl whining
>> 
>> 2. Expand tabs in touched lines (current patch)
>> 
>> 3. Expand all tabs in uri_resolve() (in a separate patch, of course)
>> 
>> 4. Expand all tabs in util/uri.c (in a separate patch, of course)
>> 
>
> Well, I never know what to do with legacy tabs, perhaps it's not worth
> polluting git blame. The code looks good, as a reviewer I'm happy to add my
>
> Reviewed-by: Fam Zheng <address@hidden>
>
> if maintainers are happy with the indentation.

In my private opinion, we should either not bother expanding tabs at
all, or we should've expanded them wholesale long ago.  Not worth
rearguing now.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]