DPDK patches and discussions
 help / color / mirror / Atom feed
From: Vlad Zolotarov <vladz@cloudius-systems.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4
Date: Tue, 14 Apr 2015 17:59:37 +0300	[thread overview]
Message-ID: <552D2B59.9000907@cloudius-systems.com> (raw)
In-Reply-To: <54963304.brPH8sEe9A@xps13>



On 04/14/15 17:17, Thomas Monjalon wrote:
> 2015-04-14 16:38, Vlad Zolotarov:
>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>> Hmmm... Unless I miss something this and one above would zero only a
>>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>>>> The original code intend to zero the whole struct. The alternative to
>>>> the original lines could be usage of memset().
>>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
>>> So I think we are ok here.
>> Yeah, I guess it does zero-initializes the rest
>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>> don't understand how the above change fixes the error if it complains
>> about the dev_info.driver_name?
> As only 1 field is required, I chose the one which should not be removed
> from this structure in the future.
>
>> What I'm trying to say - the proposed fix is completely unclear and
>> confusing. Think of somebody reading this line in a month from today -
>> he wouldn't get a clue why is it there, why to explicitly set
>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>> to add such artifacts to the code instead of just zeroing the struct
>> with a memset() and putting a good clear comment above it explaining why
>> we use a memset() and not and initializer?
> We can make it longer yes.
> I think you agree we should avoid extra lines if not needed.
> In this case, when reading "= { .field = 0 }", it seems clear our goal
> is to zero the structure (it is to me).

I'm sorry but it's not clear to me at all since the common C practice 
for zeroing the struct would be

struct st a = {0};

Like in the lines u are changing. The lines as above are clearly should 
not be commented and are absolutely clear.
The lines u are adding on the other hand are absolutely unclear and 
confusing outside the gcc bug context. Therefore it should be clearly 
stated so in a form of comment. Otherwise somebody (like myself) may see 
this and immediately fix it back (as it should be).

> I thought it is a basic C practice.

I doubt that. ;) Explained above.

>
> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> not going to comment each occurence of this coding style.
> But it must be explained in the coding style document. Agree?

OMG! This is awful! I think everybody agrees that this is a workaround 
and has nothing to do with a codding style (it's an opposite to a style 
actually). I don't know where this should be explained, frankly.

Getting back to the issue - I'm a bit surprised since I use this kind of 
initializer ({0}) in a C code for quite a long time - long before 2012. 
I'd like to understand what is a problem with this specific gcc version. 
This seems to trivial. I'm surprised CentOS has a gcc version with this 
kind of bugs.

  parent reply	other threads:[~2015-04-14 14:59 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-14  9:31 Thomas Monjalon
2015-04-14 12:48 ` Vlad Zolotarov
2015-04-14 13:06   ` Ananyev, Konstantin
2015-04-14 13:38     ` Vlad Zolotarov
2015-04-14 14:17       ` Thomas Monjalon
2015-04-14 14:30         ` Vlad Zolotarov
2015-04-14 14:53           ` Thomas Monjalon
2015-04-14 15:17             ` Vlad Zolotarov
2015-04-14 14:59         ` Vlad Zolotarov [this message]
2015-04-14 15:13           ` Thomas Monjalon
2015-04-14 15:21             ` Vlad Zolotarov
2015-04-14 15:28               ` Thomas Monjalon
2015-04-14 15:32                 ` Vlad Zolotarov
2015-04-15 20:49                 ` [dpdk-dev] [PATCH v2 1/2] " Thomas Monjalon
2015-04-15 20:49                   ` [dpdk-dev] [PATCH v2 2/2] use simple zero initializers Thomas Monjalon
2015-04-16 10:12                     ` Olivier MATZ
2015-04-16 12:55                       ` Thomas Monjalon
2015-04-16 16:31                         ` Mcnamara, John
2015-04-16  7:26                   ` [dpdk-dev] [PATCH v2 1/2] ixgbe: fix build with gcc 4.4 Zhang, Helin
2015-04-16  9:14                   ` Vlad Zolotarov
2015-04-16  9:18                     ` Thomas Monjalon
2015-04-16  9:35                       ` Vlad Zolotarov
2015-04-16 22:10                   ` [dpdk-dev] [PATCH v3 1/2] mk: fix build with gcc 4.4 and clang Thomas Monjalon
2015-04-16 22:10                     ` [dpdk-dev] [PATCH v3 2/2] use simple zero initializers Thomas Monjalon
2015-04-17 11:17                       ` Mcnamara, John
2015-04-19  8:22                       ` Vlad Zolotarov
2015-04-20 12:45                         ` Thomas Monjalon
2015-04-17 11:15                     ` [dpdk-dev] [PATCH v3 1/2] mk: fix build with gcc 4.4 and clang Mcnamara, John
2015-04-19  8:21                     ` Vlad Zolotarov
2015-04-20 12:44                       ` Thomas Monjalon
2015-04-14 12:51 ` [dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4 Vlad Zolotarov
2015-04-14 13:23   ` Ananyev, Konstantin
2015-04-14 13:41     ` Vlad Zolotarov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=552D2B59.9000907@cloudius-systems.com \
    --to=vladz@cloudius-systems.com \
    --cc=dev@dpdk.org \
    --cc=thomas.monjalon@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).