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
