Re: [Qemu-devel] [PATCH] Drop superfluous conditionals around g_strdup()

[Markus Armbruster]

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;
>>      }
>> -    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);
>> -    else if (bas->query != NULL)
>> -        res->query = g_strdup(bas->query);
>> -    if (ref->fragment != NULL)
>> -        res->fragment = g_strdup(ref->fragment);
>> +        } 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;
>>      }
>>  
>> -- 
>> 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)