checkpatch.pl report about "Missing blank line after declarations" in a structure definition

checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Dotan Barak]

Hi Joe.

Thanks for fixing the issue that I reported about.

It seems that even after your fix, there is still a false warning of "Missing blank line after declarations".

I executed checkpatch.pl on a header file that exists in the Linux tree:
# ./checkpatch.pl --file --no-tree ../include/linux/mlx5/cq.h

And got the following warning:

WARNING: Missing a blank line after declarations
#49: FILE: ../include/linux/mlx5/cq.h:49:
+       int                     irqn;
+       void (*comp)            (struct mlx5_core_cq *);


Here is the structure that checkpatch.pl is complaining about:

<snip start>
struct mlx5_core_cq {
        u32                     cqn;
        int                     cqe_sz;
        __be32                 *set_ci_db;
        __be32                 *arm_db;
        atomic_t                refcount;
        struct completion       free;
        unsigned                vector;
        int                     irqn;
        void (*comp)            (struct mlx5_core_cq *);
        void (*event)           (struct mlx5_core_cq *, enum mlx5_event);
        struct mlx5_uar        *uar;
        u32                     cons_index;
        unsigned                arm_sn;
        struct mlx5_rsc_debug   *dbg;
        int                     pid;
};
<snip end>

As you can see, this is a structure definition and IMHO the warning above is false.


Thanks!

Dotan Barak
Sr. Staff Engineer, Windows verification
Mellanox Technologies
Beit Mellanox
Yokneam, 20692

Office: +972-74-7236271
Fax:     +972-4-9593245
www.mellanox.com

Re: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Joe Perches]

On Mon, 2014-08-04 at 11:20 +0000, Dotan Barak wrote:
> Hi Joe.
> 
> Thanks for fixing the issue that I reported about.
> 
> It seems that even after your fix, there is still a false warning of "Missing blank line after declarations".
> 
> I executed checkpatch.pl on a header file that exists in the Linux tree:
> # ./checkpatch.pl --file --no-tree ../include/linux/mlx5/cq.h
> 
> And got the following warning:
> 
> WARNING: Missing a blank line after declarations
> #49: FILE: ../include/linux/mlx5/cq.h:49:
> +       int                     irqn;
> +       void (*comp)            (struct mlx5_core_cq *);
[]
> As you can see, this is a structure definition and IMHO the warning above is false.

The version in Linus' tree has a defect where
this test doesn't recognize function pointer
declarations.

This is fixed in -next since June, but it takes
awhile to get into Linus' tree.

https://lkml.org/lkml/2014/6/6/426

RE: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Dotan Barak]

Hi Joe.

The patch that you mentioned solved most of the issues, thanks!

However, there is still one more warning of this type within a struct declaration.

# ./checkpatch.pl --file --no-tree ../include/linux/mlx5/driver.h

WARNING: Missing a blank line after declarations
#508: FILE: ../include/linux/mlx5/driver.h:508:
+       struct mlx5_uuar_info   uuari;
+       MLX5_DECLARE_DOORBELL_LOCK(cq_uar_lock);


<snip start>
struct mlx5_priv {
        char                    name[MLX5_MAX_NAME_LEN];
        struct mlx5_eq_table    eq_table;
        struct mlx5_uuar_info   uuari;
        MLX5_DECLARE_DOORBELL_LOCK(cq_uar_lock);

        /* pages stuff */
        struct workqueue_struct *pg_wq;
        struct rb_root          page_root;
        int                     fw_pages;
        int                     reg_pages;
        struct list_head        free_list;

<snip end>


Thanks
Dotan

> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Monday, August 04, 2014 4:36 PM
> To: Dotan Barak
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: checkpatch.pl report about "Missing blank line after
> declarations" in a structure definition
> 
> On Mon, 2014-08-04 at 11:20 +0000, Dotan Barak wrote:
> > Hi Joe.
> >
> > Thanks for fixing the issue that I reported about.
> >
> > It seems that even after your fix, there is still a false warning of "Missing
> blank line after declarations".
> >
> > I executed checkpatch.pl on a header file that exists in the Linux tree:
> > # ./checkpatch.pl --file --no-tree ../include/linux/mlx5/cq.h
> >
> > And got the following warning:
> >
> > WARNING: Missing a blank line after declarations
> > #49: FILE: ../include/linux/mlx5/cq.h:49:
> > +       int                     irqn;
> > +       void (*comp)            (struct mlx5_core_cq *);
> []
> > As you can see, this is a structure definition and IMHO the warning above is
> false.
> 
> The version in Linus' tree has a defect where this test doesn't recognize
> function pointer declarations.
> 
> This is fixed in -next since June, but it takes awhile to get into Linus' tree.
> 
> https://lkml.org/lkml/2014/6/6/426
> 

Re: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Joe Perches]

On Mon, 2014-08-04 at 14:14 +0000, Dotan Barak wrote:
> Hi Joe.
> 
> The patch that you mentioned solved most of the issues, thanks!
> 
> However, there is still one more warning of this type within a struct declaration.
> 
> # ./checkpatch.pl --file --no-tree ../include/linux/mlx5/driver.h
> 
> WARNING: Missing a blank line after declarations
> #508: FILE: ../include/linux/mlx5/driver.h:508:
> +       struct mlx5_uuar_info   uuari;
> +       MLX5_DECLARE_DOORBELL_LOCK(cq_uar_lock);
> 
> 
> <snip start>
> struct mlx5_priv {
>         char                    name[MLX5_MAX_NAME_LEN];
>         struct mlx5_eq_table    eq_table;
>         struct mlx5_uuar_info   uuari;
>         MLX5_DECLARE_DOORBELL_LOCK(cq_uar_lock);

MLX5_DECLARE_DOORBELL is not a "normal" DECLARE macro.

Perhaps the exception for the DECLARE_<FOO> declaration
macros in the exceptions list:

	(?:$Storage\s+)?(?:DECLARE|DEFINE)_[A-Z]+\s*\(|

could be expanded to:

	(?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[A-Z]+\s*\(|

RE: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Dotan Barak]

I think that allowing a prefix is reasonable (to allow modules to add more flexibility).
I tried the suggested change and it still didn't work for me.

The following change worked for me:
(?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[A-Z_]*\s*\(|

But, maybe it is better to be even more general and allow:
(?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[ A-Z0-9_]*\s*\(|


Thanks a lot for the quick (and great) response!
Dotan

> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Monday, August 04, 2014 5:27 PM
> To: Dotan Barak
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: checkpatch.pl report about "Missing blank line after
> declarations" in a structure definition
> 
> On Mon, 2014-08-04 at 14:14 +0000, Dotan Barak wrote:
> > Hi Joe.
> >
> > The patch that you mentioned solved most of the issues, thanks!
> >
> > However, there is still one more warning of this type within a struct
> declaration.
> >
> > # ./checkpatch.pl --file --no-tree ../include/linux/mlx5/driver.h
> >
> > WARNING: Missing a blank line after declarations
> > #508: FILE: ../include/linux/mlx5/driver.h:508:
> > +       struct mlx5_uuar_info   uuari;
> > +       MLX5_DECLARE_DOORBELL_LOCK(cq_uar_lock);
> >
> >
> > <snip start>
> > struct mlx5_priv {
> >         char                    name[MLX5_MAX_NAME_LEN];
> >         struct mlx5_eq_table    eq_table;
> >         struct mlx5_uuar_info   uuari;
> >         MLX5_DECLARE_DOORBELL_LOCK(cq_uar_lock);
> 
> MLX5_DECLARE_DOORBELL is not a "normal" DECLARE macro.
> 
> Perhaps the exception for the DECLARE_<FOO> declaration macros in the
> exceptions list:
> 
> 	(?:$Storage\s+)?(?:DECLARE|DEFINE)_[A-Z]+\s*\(|
> 
> could be expanded to:
> 
> 	(?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[A-Z]+\s*\(|
> 

Re: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Joe Perches]

On Mon, 2014-08-04 at 14:50 +0000, Dotan Barak wrote:
> I think that allowing a prefix is reasonable (to allow modules to add more flexibility).
> I tried the suggested change and it still didn't work for me.
> 
> The following change worked for me:
> (?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[A-Z_]*\s*\(|
> 
> But, maybe it is better to be even more general and allow:
> (?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[ A-Z0-9_]*\s*\(|

Maybe:

(?:$Storage\s+)(?:[A-Z_][A-Z0-9]+_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-9]+){1,2}

RE: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Dotan Barak]

In our code (I take it as an example), we used  MLX5_DECLARE_DOORBELL_LOCK,
So I guess that the regular expression that you mentioned will fail on it
(because the "_" is not allowed to be repeated).

So, I would change it a little bit to allow people to use underscore as a word separator.

Thanks
Dotan

> >
> > The following change worked for me:
> > (?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[A-Z_]*\s*\(|
> >
> > But, maybe it is better to be even more general and allow:
> > (?:$Storage\s+)?[A-Z0-9_]*(?:DECLARE|DEFINE)_[ A-Z0-9_]*\s*\(|
> 
> Maybe:
> 
> (?:$Storage\s+)(?:[A-Z_][A-Z0-9]+_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-
> 9]+){1,2}
> 

Re: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Joe Perches]

On Mon, 2014-08-04 at 15:19 +0000, Dotan Barak wrote:
> In our code (I take it as an example), we used  MLX5_DECLARE_DOORBELL_LOCK,
> So I guess that the regular expression that you mentioned will fail on it
> (because the "_" is not allowed to be repeated).
> 
> So, I would change it a little bit to allow people to use underscore as a word separator.
[]
> > Maybe:
> > 
> > (?:$Storage\s+)(?:[A-Z_][A-Z0-9]+_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-9]+){1,2}

What I suggested should allow any of:

	DECLARE_FOO
	_FOO_DEFINE_BAR
	FOO_BAR_DEFINE_BAZ
	FOO_BAR_DECLARE_BAZ_QUX

where FOO/BAR/BAZ/QUX can also have digits

RE: checkpatch.pl report about "Missing blank line after declarations" in a structure definition

[Dotan Barak]

(I guess I need to refresh my regular expression skills ...)

Thanks, this will be just great!
Dotan

> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Monday, August 04, 2014 6:27 PM
> To: Dotan Barak
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: checkpatch.pl report about "Missing blank line after
> declarations" in a structure definition
> 
> On Mon, 2014-08-04 at 15:19 +0000, Dotan Barak wrote:
> > In our code (I take it as an example), we used
> > MLX5_DECLARE_DOORBELL_LOCK, So I guess that the regular expression
> > that you mentioned will fail on it (because the "_" is not allowed to be
> repeated).
> >
> > So, I would change it a little bit to allow people to use underscore as a word
> separator.
> []
> > > Maybe:
> > >
> > > (?:$Storage\s+)(?:[A-Z_][A-Z0-9]+_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-
> > > 9]+){1,2}
> 
> What I suggested should allow any of:
> 
> 	DECLARE_FOO
> 	_FOO_DEFINE_BAR
> 	FOO_BAR_DEFINE_BAZ
> 	FOO_BAR_DECLARE_BAZ_QUX
> 
> where FOO/BAR/BAZ/QUX can also have digits