Re: i915 vs checkpatch

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Mar 01, 2018 at 12:43:22PM +0200, Jani Nikula wrote:
> On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
> > Since not so long ago our CI is running and reporting sparse and
> > checkpatch. Sparse is doing just fine but I had to disable checkpatch
> > for the time being - too much "false" positives causing people to
> > complain. It's simply confusing to see one thing in the code, and
> > fitting your change in only to get a report that it's wrong.
> >
> > We are explicitly going against couple of the recommendations it tries
> > to enforce, e.g. not using BIT macro, splitting quoted strings:
> > https://lists.freedesktop.org/archives/intel-gfx/2018-February/156613.html
> >
> > IMHO we should make a couple of decisions here:
> >  1. Do we really want to use the checkpatch / have CI reports?
> 
> I think yes, for the benefit of both patch authors and reviewers. For
> the most part, we do want to encourage uniform style.
> 
> >  2. Which of the checkpatch checks we want to disabled for i915?
> 
> One low hanging fruit is to ignore the CHECK messages, or drop the
> --strict option to checkpatch.pl in CI, although I think some of them
> are nice.

IMO the strict vs. not split is totally arbitrary. Some really obviously
correct warning are only enabled with strict, whereas even w/o strict
you get some warnings that are just plain silly. Thus I think strict
does more good than harm.

> 
> >  3. How strongly do we want to enforce the rest?
> 
> That's a tough one. With code movement, you want the code to remain the
> same instead of changing at the same time. And some of the warnings are
> subjective. For example, I'd prefer to stick with the 80 column rule but
> only when it makes sense. ;)
> 
> Another example, I would like to move towards kernel types over uint8_t
> and friends. However, when you have code surrounded by uint8_t and
> friends, it's often better to stick with the style around you.
> 
> >  4. Do we want to change what's already in the tree, for compliance?
> 
> No. I don't think we should encourage mindless checkpatch fixes.
> 
> Does checkpatch support disabling checks or do you have to filter them
> out from the output?
> 
> BR,
> Jani.
> 
> 
> >
> > Recent-ish drm-tip, vanilla checkpatch on i915 reports:
> > total: 399 errors, 3573 warnings, 209374 lines checked
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dim-tools mailing list
> dim-tools@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx