From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to; bh=b5uzgFZrgTE/kVaA2JkKb1fBwEMauKunWVxgv2dqVLA=; b=eMY5Nm0uxOrE4A1vco3oF3ywdbVrwFnHXm0gIVTKawJf2a+EH+BFNnJNAuSh15m55A cF7Eu4MlXZvmxJCd+KNcG9Sjie5CYdZ+0iDLnLGJ0GWMKbybnmFvzGjq+6KpFARRMs9V qEKW8eg434+cUsSu4ZvTbrLhzB8jK1VHO2eTE9cqTVCCugc1ugOflouXyuRGIAr14TR/ ZNZgmEt9MQL0q0A/SBmsMLZPN6mMMmxDJ72tair2UvopwTULkqG76Vi4zZwFXHVebXtX zF/FyApPVlQmZ2zlc9oYoQeFNMwwaAc2kpkM+mF1Dd+O9LCandtDVgMuGTtyF+9+zuuW Ozpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to; bh=b5uzgFZrgTE/kVaA2JkKb1fBwEMauKunWVxgv2dqVLA=; b=DEeDyZggKFxwN912CImUv2I/Cyc7bS4ZvJobedF59ywWLR058irGEKE3dU24VA1aLk xY0Fe0OmKKtQBlCSek3RrsfQnMql98C9zr+zH7kveMv/4vka7p4VX0R4487a1EHrck/g BMBkUmAUfn2fbQpSe1GCZCvNpmHxfThYe2NVVstsqLwnuCqdvZ93mk413h2IZEutQHsX PfDwijUsS2G8Zy6IHrGhPIsUaf2dmQqMeDnPXCmFFFHGNfdl+ncBxIGthxNlGGzUPfku Ko0Y16EiuUgYU/ywS+/04Yt74krVCXf2o2oB15zVk9OrsPnsdEH5rUC9t+a/M3hS88Q1 1iww== X-Gm-Message-State: AG10YOR96EHg9dC1qvMZLEZlolZL0kstm3Tn71ZIVNptvY/a6L1bux+cg0CWtt8FVb/Ro/Im X-Received: by 10.28.210.73 with SMTP id j70mr18849383wmg.8.1456249123541; Tue, 23 Feb 2016 09:38:43 -0800 (PST) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id w66sm27204766wmd.2.2016.02.23.09.38.42 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 23 Feb 2016 09:38:42 -0800 (PST) Date: Tue, 23 Feb 2016 18:38:28 +0100 From: Adrien Mazarguil To: Thomas Monjalon Message-ID: <20160223173828.GY27079@6wind.com> Mail-Followup-To: Thomas Monjalon , Bruce Richardson , dev@dpdk.org 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 Content-Disposition: inline In-Reply-To: <2522248.H1QVqHKcgq@xps13> Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 4/5] mlx5: add support for flow director X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Feb 2016 17:38:43 -0000 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