From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 4/5] mlx5: add support for flow director Date: Tue, 23 Feb 2016 18:38:28 +0100 Message-ID: <20160223173828.GY27079@6wind.com> References: <1454063522-1948-1-git-send-email-adrien.mazarguil@6wind.com> <20160218161016.GQ27079@6wind.com> <20160223151304.GA17644@bricha3-MOBL3> <2522248.H1QVqHKcgq@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: Thomas Monjalon Return-path: Received: from mail-wm0-f53.google.com (mail-wm0-f53.google.com [74.125.82.53]) by dpdk.org (Postfix) with ESMTP id A9A2D2A6C for ; Tue, 23 Feb 2016 18:38:43 +0100 (CET) Received: by mail-wm0-f53.google.com with SMTP id b205so2503766wmb.1 for ; Tue, 23 Feb 2016 09:38:43 -0800 (PST) Content-Disposition: inline In-Reply-To: <2522248.H1QVqHKcgq@xps13> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Feb 23, 2016 at 06:14:19PM +0100, Thomas Monjalon wrote: > 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?). Actually you can ignore this one and NULL comparison checks - they do not show up when using scripts/checkpatches.sh (I was previously using checkpatch.pl directly). I've fixed and sent updated versions of these patchsets anyway, with errno warnings still present since it's a design decision. We can discuss it if necessary. > Other checks deserve to be followed more strictly (e.g. negative errno). -- Adrien Mazarguil 6WIND