Re: [dpdk-dev] [PATCH 4/5] mlx5: add support for flow director

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-02-23 15:13, Bruce Richardson:
> On Thu, Feb 18, 2016 at 05:10:16PM +0100, Adrien Mazarguil wrote:
> > Hi Bruce,
> > 
> > On Wed, Feb 17, 2016 at 05:13:44PM +0000, Bruce Richardson wrote:
> > > Hi Adrien, Yaacov,
> > > 
> > > this patch raises a lot of warnings (17) with checkpatch. Can you perhaps look
> > > to see if this number can be reduced.
> > 
> > We actually did it before submitting that patch, there is indeed a bunch of
> > remaining warnings that have been left on purpose. Not sure if we have the
> > same configuration for checkpatch, but they should fall into the following
> > categories:
> > 
> > - "WARNING: return of an errno should typically be negative" - All return
> >   values are documented in the code. Since this PMD uses syscalls in its
> >   control path, it uses positive errno values internally for
> >   consistency. Public callback functions however return negative error
> >   values.
> > 
> > - "WARNING: line over 80 characters" - Well, although I'm a big fan of the
> >   80 characters limit, breaking those would have made the code harder to
> >   read.
> > 
> > - "WARNING: Missing a blank line after declarations" - It's actually a
> >   declaration through a macro, there is no missing blank line.
> > 
> > - "WARNING: networking block comments don't use an empty /* line" - Not sure
> >   if we really care? I don't particularly mind.
> > 
> > - "CHECK: Comparison to NULL could be written "!" - I do not mind either,
> >   writing the full check seems clearer to me.
> > 
> > - "CHECK: Unnecessary parentheses around fdir_info->mask" - Looks like a
> >   valid, although minor error.
> > 
> > Please tell me which of these still need to be fixed.
> > 
> Hi Adrien,
> 
> sorry for the delayed reply, I was out for a couple of days.
> 
> As none of the above are errors, I'm not going to mandate that they be fixed
> before I merge in the patch, so long as you as maintainer are happy with them.
> 
> My request mainly came about because of the sheer number of warnings that were
> being flagged. To keep the codebase clean requires constant discipline, so I
> don't like seeing patches where 17 warnings are flagged. I was hoping since
> a new rev of the set was likely anyway that some steps could be taken to reduce
> that number.
> 
> Thomas, any thoughts here, since I'm still "learning the ropes" as committer. 
> Do you have any rules-of-thumb or guidelines as regards checkpatch warnings? The
> contributor guide only seems to cover running checkpatch, not anything about
> what to do with the output.

I totally agree with you, Bruce.
Everybody must make some effort to keep consistency and avoid coding style
exceptions. Some code areas are not yet fully compliant with the rules,
depending of their history and... their maintainers ;)
I think we can tolerate some exceptions like for the 80 char limit.
Some checks may be disabled after discussion (networking block comments?).
Other checks deserve to be followed more strictly (e.g. negative errno).