Re: i915 vs checkpatch

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 02 Mar 2018, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> Quoting Rodrigo Vivi (2018-03-01 20:00:07)
>> On Thu, Mar 01, 2018 at 06:13:31PM +0200, Jani Nikula wrote:
>> > 
>> > I went through the recent checkpatch reports, and here's my take.
>> > 
>> > On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
>> > >  2. Which of the checkpatch checks we want to disabled for i915?
>> > 
>> > I'd like to have these silenced:
>> > 
>> > CHECK: No space is necessary after a cast
>> > WARNING: line over 80 characters
>> > WARNING: quoted string split across lines
>> > 
>> > I'd prefer we conform to the last two too, but there's just too much
>> > noise and too many cases where we explicitly should ignore them.
>> > 
>> > For the time being, I think we may have to silence these ones too, but
>> > I'd like us to discuss enforcing them:
>> > 
>> > CHECK: Prefer kernel type 'u16' over 'uint16_t'
>> > CHECK: Prefer kernel type 'u32' over 'uint32_t'
>> > CHECK: Prefer kernel type 'u64' over 'uint64_t'
>> > CHECK: Prefer kernel type 'u8' over 'uint8_t'
>> > CHECK: Prefer using the BIT macro
>> > 
>> > The BIT macros is one that I'd consider accepting a one-time conversion
>> > of i915_reg.h and after that use it exclusively. But up to debate.
>> 
>> For this one I just wonder if we would need to do a massive
>> change before. Because it would get ugly to have mixed cases.
>
> Yep, the mixed cases are bit tough to automatically enforce. So the
> transitional phase will always be troublesome, and trying to make that
> shorter makes some sense to me.
>
> Traditionally we've avoided mass changes just for the changes, but we
> have to assess the value of doing it against what we get. That is
> getting automatic enforcement, once we've converted over.
>
> We're not that far off the mark with u(32|16|8) vs uint(32|16|8)_t:
>
> i915$ git grep -E "uint(32|16|8)_t" | wc -l
> 852
> i915$ git grep -E "u(32|16|8)" | wc -l
> 3857

Doing a bit of git grep -cE with those seems to indicate that code with
uint(32|16|8)_t promotes *more* use of those. It's natural and it goes
by the rule of following the style surrounding your changes.

> I don't consider that undoable.

It's doable. Only a question of whether we want to do that or not.

> BIT() is in the minority at the moment, so it might benefit even more as
> people often cargo-cult the programming style from other places in the code.

FWIW I think BIT(n) is simply better than open-coding (1 << n), while
the kernel vs. C types is more of an aestheical thing.

> I think it might be worthy doing these two changes to get the automatic
> enforemend and avoid the codebase staying in limbo. Machine overlords are
> way better at enforcing any code checks than us humans.

Agreed on the machine overlords.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx