2011-02-21 23:28:09

by Corey Ashford

Subject: [BUG ?] checkpatch.pl rejects as error something I think it ought to be allow

Hi,

I have a piece of code where I have two constants defined as follows:

static const unsigned long polling_interval_sec = 1;
static const unsigned long polling_interval_ns = 0;

Now, it's clear to me that I want these two values to have the keywords
const and static. I could use a #define here, but const static seemed
cleaner to me.

When I run checkpatch.pl across this code, I get this error:

ERROR: do not initialise statics to 0 or NULL.

I think the problem here is that another case is needed for "static
const" that does allow 0.

What do you think?

Thanks for your consideration,

- Corey

2011-02-21 23:40:27

by Andy Whitcroft

Subject: Re: [BUG ?] checkpatch.pl rejects as error something I think it ought to be allow

On Mon, Feb 21, 2011 at 03:28:02PM -0800, Corey Ashford wrote:
> Hi,
>
> I have a piece of code where I have two constants defined as follows:
>
> static const unsigned long polling_interval_sec = 1;
> static const unsigned long polling_interval_ns = 0;
>
> Now, it's clear to me that I want these two values to have the
> keywords const and static. I could use a #define here, but const
> static seemed cleaner to me.
>
> When I run checkpatch.pl across this code, I get this error:
>
> ERROR: do not initialise statics to 0 or NULL.
>
> I think the problem here is that another case is needed for "static
> const" that does allow 0.
>
> What do you think?
>
> Thanks for your consideration,

The warning is intended to tell you that the = 0 is unnecessary. Any
static is 0 by default I believe. At some point the addition of the 0
would move the value from the bss to the data segment bloating the code.
This may no longer be true.

-apw

2011-02-21 23:49:50

by Corey Ashford

Subject: Re: [BUG ?] checkpatch.pl rejects as error something I think it ought to be allow

On 02/21/2011 03:40 PM, Andy Whitcroft wrote:
> On Mon, Feb 21, 2011 at 03:28:02PM -0800, Corey Ashford wrote:
>> Hi,
>>
>> I have a piece of code where I have two constants defined as follows:
>>
>> static const unsigned long polling_interval_sec = 1;
>> static const unsigned long polling_interval_ns = 0;
>>
>> Now, it's clear to me that I want these two values to have the
>> keywords const and static. I could use a #define here, but const
>> static seemed cleaner to me.
>>
>> When I run checkpatch.pl across this code, I get this error:
>>
>> ERROR: do not initialise statics to 0 or NULL.
>>
>> I think the problem here is that another case is needed for "static
>> const" that does allow 0.
>>
>> What do you think?
>>
>> Thanks for your consideration,
>
> The warning is intended to tell you that the = 0 is unnecessary. Any
> static is 0 by default I believe. At some point the addition of the 0
> would move the value from the bss to the data segment bloating the code.
> This may no longer be true.

OK, but that means I'd have to have a declaration like this, which looks
quite odd to me:

static const poll_interval_ns; /* = 0 */

I don't think that is preferable to this:

static const poll_interval_ns = 0;

- Corey

2011-02-22 09:07:24

by Andy Whitcroft

Subject: Re: [BUG ?] checkpatch.pl rejects as error something I think it ought to be allow

On Mon, Feb 21, 2011 at 03:49:33PM -0800, Corey Ashford wrote:
> On 02/21/2011 03:40 PM, Andy Whitcroft wrote:
> >On Mon, Feb 21, 2011 at 03:28:02PM -0800, Corey Ashford wrote:
> >>Hi,
> >>
> >>I have a piece of code where I have two constants defined as follows:
> >>
> >> static const unsigned long polling_interval_sec = 1;
> >> static const unsigned long polling_interval_ns = 0;
> >>
> >>Now, it's clear to me that I want these two values to have the
> >>keywords const and static. I could use a #define here, but const
> >>static seemed cleaner to me.
> >>
> >>When I run checkpatch.pl across this code, I get this error:
> >>
> >>ERROR: do not initialise statics to 0 or NULL.
> >>
> >>I think the problem here is that another case is needed for "static
> >>const" that does allow 0.
> >>
> >>What do you think?
> >>
> >>Thanks for your consideration,
> >
> >The warning is intended to tell you that the = 0 is unnecessary. Any
> >static is 0 by default I believe. At some point the addition of the 0
> >would move the value from the bss to the data segment bloating the code.
> >This may no longer be true.
>
> OK, but that means I'd have to have a declaration like this, which
> looks quite odd to me:
>
> static const poll_interval_ns; /* = 0 */
>
> I don't think that is preferable to this:
>
> static const poll_interval_ns = 0;

Well I'd not bother with the comment as that is defined behaviour. But I
suspect that gcc has gained the smarts to not break these and perhaps the
check is long in the tooth. As for this patch, as checkpatch is intended
to be an advisory helper you are free to ignore its recommendations,
just expect to be asked about it by any human reviewers.

-apw