Re: i915 vs checkpatch

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

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.

>  3. How strongly do we want to enforce the rest?

It depends on the case. Some of the warnings are notes to check, will be
emitted even for correct code, and can't be automatically enforced.

Of the recently reported ones, I'd like to enforce:

CHECK: Alignment should match open parenthesis
CHECK: Blank lines aren't necessary after an open brace '{'
CHECK: Lines should not end with a '('
CHECK: Please don't use multiple blank lines
CHECK: Please use a blank line after function/struct/union/enum declarations
CHECK: Unbalanced braces around else statement
CHECK: Unnecessary parentheses around 'level >= 1'
CHECK: Unnecessary parentheses around 'pipe == PIPE_A'
CHECK: braces {} should be used on all arms of this statement
CHECK: multiple assignments should be avoided
CHECK: spaces preferred around that '*' (ctx:VxV)
CHECK: spaces preferred around that '<<' (ctx:VxV)
CHECK: spinlock_t definition without comment
CHECK: struct mutex definition without comment
ERROR: Missing Signed-off-by: line(s)
ERROR: Unrecognized email address: Foo Bar <foo.bar@example.com'
ERROR: code indent should use tabs where possible
ERROR: spaces required around that '=' (ctx:VxW)
ERROR: trailing whitespace
WARNING: 'consistancy' may be misspelled - perhaps 'consistency'?
WARNING: 'writting' may be misspelled - perhaps 'writing'?
WARNING: Missing a blank line after declarations
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
WARNING: Statements should start on a tabstop
WARNING: please, no space before tabs
WARNING: please, no spaces at the start of a line
WARNING: suspect code indent for conditional statements (8, 17)

These ones should be double checked by author/reviewer/committer. These
can't be automatically enforced:

CHECK: Macro argument reuse 'info' - possible side-effects?
ERROR: Macros with complex values should be enclosed in parentheses
WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
WARNING: Macros with flow control statements should be avoided
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

If we have the filtering in place in dim, we could require the committer
to pass a parameter to dim to apply patches with the above warnings.

>  4. Do we want to change what's already in the tree, for compliance?

With the exception of the BIT() macro, I still think not. But patch
series touching existing code should move towards compliance (for want
of a better word).


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