Re: i915 vs checkpatch

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Arkadiusz Hiler <arkadiusz.hiler@intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, dim-tools@lists.freedesktop.org
Subject: Re: i915 vs checkpatch
Date: Thu, 01 Mar 2018 12:43:22 +0200	[thread overview]
Message-ID: <87371kuldh.fsf@intel.com> (raw)
In-Reply-To: <20180301094706.GA27356@ahiler-desk1.ger.corp.intel.com>

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.

>  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx