Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

[Fengguang Wu]

Hi Eric,

On Sun, Jul 07, 2013 at 10:14:09PM -0500, Eric Van Hensbergen wrote:
> I've fixed this in my for-next branch, Linus rejected my pull on other
> grounds so this should let us fix up these style issues before the merge.
>  Thanks for catching this, I need to go back to running
> checkpatch.plbefore I pull into my tree.  Jim, please run
> checkpatch.pl before sending the patches in the future.  Its annoying and
> there are false positives, but we need to keep things consistent.

Thanks for fixing the style warnings! In long run I'd like to disable
the check types that tend to have false positives.  As for now, these
checks are disabled in my checkpatch robot:

PATCH_PREFIX
LONG_LINE
CAMELCASE
MISSING_SIGN_OFF
COMPLEX_MACRO
BRACES
SPACING
LONG_LINE
REDUNDANT_CODE
TRAILING_STATEMENTS
UNCOMMENTED_DEFINITION

Hopefully this can remind people to run scripts/checkpatch.pl earlier
when patches are still in their hands. :)

Thanks,
Fengguang

> On Sun, Jul 7, 2013 at 7:42 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
>
> >
> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/ericvh/v9fs.gitfor-linus
> > head:   ceb251d27e4f1001f3b3feca2e2c774f582e195d
> > commit: ceb251d27e4f1001f3b3feca2e2c774f582e195d [7/7] fs/9p: xattr: add
> > trusted and security namespaces
> >
> > scripts/checkpatch.pl0001-fs-9p-xattr-add-trusted-and-security-namespaces.patch
> >
> > ERROR: code indent should use tabs where possible
> > #84: FILE: fs/9p/xattr.c:176:
> > +        &v9fs_xattr_security_handler,$
> >
> > WARNING: please, no spaces at the start of a line
> > #84: FILE: fs/9p/xattr.c:176:
> > +        &v9fs_xattr_security_handler,$
> >
> > ---
> > 0-DAY kernel build testing backend              Open Source Technology
> > Center
> > http://lists.01.org/mailman/listinfo/kbuild                 Intel
> > Corporation
> >

Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

[Joe Perches]

On Mon, 2013-07-08 at 11:30 +0800, Fengguang Wu wrote:
> In long run I'd like to disable
> the check types that tend to have false positives.  As for now, these
> checks are disabled in my checkpatch robot:

Are you using a .checkpatch.conf file?

> PATCH_PREFIX
> LONG_LINE
> CAMELCASE

That's a --strict test

> MISSING_SIGN_OFF

Why would you even consider this one?

> COMPLEX_MACRO
> BRACES
> SPACING
> LONG_LINE

OK by me.

> REDUNDANT_CODE

Also a --strict test

> TRAILING_STATEMENTS

False positives?  Got an example?

> UNCOMMENTED_DEFINITION

Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

[Fengguang Wu]

On Sun, Jul 07, 2013 at 08:54:04PM -0700, Joe Perches wrote:
> On Mon, 2013-07-08 at 11:30 +0800, Fengguang Wu wrote:
> > In long run I'd like to disable
> > the check types that tend to have false positives.  As for now, these
> > checks are disabled in my checkpatch robot:
> 
> Are you using a .checkpatch.conf file?

Nope.

> > PATCH_PREFIX
> > LONG_LINE
> > CAMELCASE
> 
> That's a --strict test

Yes. The CAMELCASE checks are false positives for the USB subsystem.
Where they followed the USB spec and use names like bInterfaceClass
all over the places.

> > MISSING_SIGN_OFF
> 
> Why would you even consider this one?

It produces

        ERROR: Missing Signed-off-by: line(s)

on
        commit: 8bb495e3f02401ee6f76d1b1d77f3ac9f079e376 Linux 3.10

        tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git master
        commit: 97966e7ec944580bc3f46712f34ecb3c854fdd4b [37/81] Revert "ALSA: hda - Fix wrong power setup for HP paths of VIA codecs"

        tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git master
        head:   120b1922175789a8a9bdafb7ea755fc63de2f392
        commit: 10296279acc5d3caeca7b2cfde329c8280fe67e5 [32/81] Merge branch 'for-linus'

and many other commits

> > COMPLEX_MACRO
> > BRACES
> > SPACING
> > LONG_LINE
> 
> OK by me.
> 
> > REDUNDANT_CODE
> 
> Also a --strict test
> 
> > TRAILING_STATEMENTS
> 
> False positives?  Got an example?

tree:   git://git.freedesktop.org/git/nouveau/linux-2.6 drm-nouveau-next
head:   d2989b534ef6834ebf2425aecc040b894b567c91
commit: 01672ef454307bf63e93defb3599399b678ff58b [3/68] drm/nve0/fifo: copy engine context stored in ramfc, not externally

ERROR: trailing statements should be on next line
#28: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:146:
+       case NVDEV_ENGINE_GR   : addr = 0x0210; break;

ERROR: trailing statements should be on next line
#40: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:183:
+       case NVDEV_ENGINE_COPY2: addr = 0x0000; break;

ERROR: trailing statements should be on next line
#41: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:184:
+       case NVDEV_ENGINE_GR   : addr = 0x0210; break;
 
> > UNCOMMENTED_DEFINITION

Thanks,
Fengguang

Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

[Fengguang Wu]

> > > TRAILING_STATEMENTS
> > 
> > False positives?  Got an example?
> 
> tree:   git://git.freedesktop.org/git/nouveau/linux-2.6 drm-nouveau-next
> head:   d2989b534ef6834ebf2425aecc040b894b567c91
> commit: 01672ef454307bf63e93defb3599399b678ff58b [3/68] drm/nve0/fifo: copy engine context stored in ramfc, not externally
> 
> ERROR: trailing statements should be on next line
> #28: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:146:
> +       case NVDEV_ENGINE_GR   : addr = 0x0210; break;
> 
> ERROR: trailing statements should be on next line
> #40: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:183:
> +       case NVDEV_ENGINE_COPY2: addr = 0x0000; break;
> 
> ERROR: trailing statements should be on next line
> #41: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:184:
> +       case NVDEV_ENGINE_GR   : addr = 0x0210; break;

In this case the code does look neat:

        switch (nv_engidx(object->engine)) {
        case NVDEV_ENGINE_SW   : return 0;
-       case NVDEV_ENGINE_GR   :
        case NVDEV_ENGINE_COPY0:
-       case NVDEV_ENGINE_COPY1: addr = 0x0210; break;
+       case NVDEV_ENGINE_COPY1:
+       case NVDEV_ENGINE_COPY2: addr = 0x0000; break;
+       case NVDEV_ENGINE_GR   : addr = 0x0210; break;
        case NVDEV_ENGINE_BSP  : addr = 0x0270; break;
        case NVDEV_ENGINE_VP   : addr = 0x0250; break;
        case NVDEV_ENGINE_PPP  : addr = 0x0260; break;

Thanks,
Fengguang

Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

[Fengguang Wu]

On Mon, Jul 08, 2013 at 12:47:47PM +0800, Fengguang Wu wrote:
> > > > TRAILING_STATEMENTS
> > > 
> > > False positives?  Got an example?
> > 
> > tree:   git://git.freedesktop.org/git/nouveau/linux-2.6 drm-nouveau-next
> > head:   d2989b534ef6834ebf2425aecc040b894b567c91
> > commit: 01672ef454307bf63e93defb3599399b678ff58b [3/68] drm/nve0/fifo: copy engine context stored in ramfc, not externally
> > 
> > ERROR: trailing statements should be on next line
> > #28: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:146:
> > +       case NVDEV_ENGINE_GR   : addr = 0x0210; break;
> > 
> > ERROR: trailing statements should be on next line
> > #40: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:183:
> > +       case NVDEV_ENGINE_COPY2: addr = 0x0000; break;
> > 
> > ERROR: trailing statements should be on next line
> > #41: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:184:
> > +       case NVDEV_ENGINE_GR   : addr = 0x0210; break;
> 
> In this case the code does look neat:
> 
>         switch (nv_engidx(object->engine)) {
>         case NVDEV_ENGINE_SW   : return 0;
> -       case NVDEV_ENGINE_GR   :
>         case NVDEV_ENGINE_COPY0:
> -       case NVDEV_ENGINE_COPY1: addr = 0x0210; break;
> +       case NVDEV_ENGINE_COPY1:
> +       case NVDEV_ENGINE_COPY2: addr = 0x0000; break;
> +       case NVDEV_ENGINE_GR   : addr = 0x0210; break;
>         case NVDEV_ENGINE_BSP  : addr = 0x0270; break;
>         case NVDEV_ENGINE_VP   : addr = 0x0250; break;
>         case NVDEV_ENGINE_PPP  : addr = 0x0260; break;

It's actually a common case.

        git grep ';.*;$' -- '*.c' | grep -o '\S*$' | sort | uniq -c | sort -nr | head

==>        4156 break;
            465 return;);
            273 -1;);
            158 0;
            146 mdelay(1);
            143 eieio();
            140 msleep(1);
            135 4;
             94 ;
             93 8;

Thanks,
Fengguang

Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

[Joe Perches]

On Mon, 2013-07-08 at 12:45 +0800, Fengguang Wu wrote:
> On Sun, Jul 07, 2013 at 08:54:04PM -0700, Joe Perches wrote:
> > On Mon, 2013-07-08 at 11:30 +0800, Fengguang Wu wrote:
> > > In long run I'd like to disable
> > > the check types that tend to have false positives.  As for now, these
> > > checks are disabled in my checkpatch robot:
> > 
> > Are you using a .checkpatch.conf file?
> 
> Nope.
> 
> > > PATCH_PREFIX
> > > LONG_LINE
> > > CAMELCASE
> > 
> > That's a --strict test
> 
> Yes. The CAMELCASE checks are false positives for the USB subsystem.
> Where they followed the USB spec and use names like bInterfaceClass
> all over the places.

The latest version of checkpatch should avoid this.

> > > MISSING_SIGN_OFF
> > 
> > Why would you even consider this one?
> 
> It produces
> 
>         ERROR: Missing Signed-off-by: line(s)
> 
> on
>         commit: 8bb495e3f02401ee6f76d1b1d77f3ac9f079e376 Linux 3.10

Well, clearly Linus should sign off on his own commits...

>         tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git master
>         commit: 97966e7ec944580bc3f46712f34ecb3c854fdd4b [37/81] Revert "ALSA: hda - Fix wrong power setup for HP paths of VIA codecs"
> 
>         tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git master
>         head:   120b1922175789a8a9bdafb7ea755fc63de2f392
>         commit: 10296279acc5d3caeca7b2cfde329c8280fe67e5 [32/81] Merge branch 'for-linus'
> 
> and many other commits

Not in -next as of today.

> > > TRAILING_STATEMENTS
> > False positives?  Got an example?
> tree:   git://git.freedesktop.org/git/nouveau/linux-2.6 drm-nouveau-next
> head:   d2989b534ef6834ebf2425aecc040b894b567c91
> commit: 01672ef454307bf63e93defb3599399b678ff58b [3/68] drm/nve0/fifo: copy engine context stored in ramfc, not externally
> 
> ERROR: trailing statements should be on next line
> #28: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:146:
> +       case NVDEV_ENGINE_GR   : addr = 0x0210; break;

Not false positives.

Re: [v9fs:for-linus 7/7] ERROR: code indent should use tabs where possible

[Fengguang Wu]

Hi Joe,

On Sun, Jul 07, 2013 at 09:54:27PM -0700, Joe Perches wrote:
> On Mon, 2013-07-08 at 12:45 +0800, Fengguang Wu wrote:
> > On Sun, Jul 07, 2013 at 08:54:04PM -0700, Joe Perches wrote:
> > > On Mon, 2013-07-08 at 11:30 +0800, Fengguang Wu wrote:
> > > > In long run I'd like to disable
> > > > the check types that tend to have false positives.  As for now, these
> > > > checks are disabled in my checkpatch robot:
> > > 
> > > Are you using a .checkpatch.conf file?
> > 
> > Nope.
> > 
> > > > PATCH_PREFIX
> > > > LONG_LINE
> > > > CAMELCASE
> > > 
> > > That's a --strict test
> > 
> > Yes. The CAMELCASE checks are false positives for the USB subsystem.
> > Where they followed the USB spec and use names like bInterfaceClass
> > all over the places.
> 
> The latest version of checkpatch should avoid this.

That'd be great!  So I'll use the latest version and take the
CAMELCASE check back.

> > > > MISSING_SIGN_OFF
> > > 
> > > Why would you even consider this one?
> > 
> > It produces
> > 
> >         ERROR: Missing Signed-off-by: line(s)
> > 
> > on
> >         commit: 8bb495e3f02401ee6f76d1b1d77f3ac9f079e376 Linux 3.10
> 
> Well, clearly Linus should sign off on his own commits...

Heh.

> >         tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git master
> >         commit: 97966e7ec944580bc3f46712f34ecb3c854fdd4b [37/81] Revert "ALSA: hda - Fix wrong power setup for HP paths of VIA codecs"
> > 
> >         tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git master
> >         head:   120b1922175789a8a9bdafb7ea755fc63de2f392
> >         commit: 10296279acc5d3caeca7b2cfde329c8280fe67e5 [32/81] Merge branch 'for-linus'
> > 
> > and many other commits
> 
> Not in -next as of today.

Since I test bleeding edge trees and many of them are not formal enough, I'm
afraid I have to ignore the missing Signed-off-by lines..

10071   F Jul 05 Cc fengguang.wu (  37:0) [asoc:topic/ak4554 0/2] ERROR: Missing Signed-off-by: line(s)
10087   F Jul 05 Cc fengguang.wu (  36:0) [sound:master 32/81] ERROR: Missing Signed-off-by: line(s)
10088   F Jul 05 Cc fengguang.wu (  36:0) [sound:master 37/81] ERROR: Missing Signed-off-by: line(s)
10116   F Jul 06 Cc fengguang.wu (  37:0) [x0148406:tmp 0/45] ERROR: Missing Signed-off-by: line(s)
10117   F Jul 06 Cc fengguang.wu (  37:0) [x0148406:tmp 1/45] ERROR: Missing Signed-off-by: line(s)
10118   F Jul 06 Cc fengguang.wu (  37:0) [x0148406:tmp 2/45] ERROR: Missing Signed-off-by: line(s)
10119   F Jul 06 Cc fengguang.wu (  37:0) [x0148406:tmp 3/45] ERROR: Missing Signed-off-by: line(s)
10120   F Jul 06 Cc fengguang.wu (  37:0) [mmc:mmc-next 0/88] ERROR: Missing Signed-off-by: line(s)
10183   F Jul 07 Cc fengguang.wu (  37:0) [target:target-per-cpu-ida 0/58] ERROR: Missing Signed-off-by: line(s)

> > > > TRAILING_STATEMENTS
> > > False positives?  Got an example?
> > tree:   git://git.freedesktop.org/git/nouveau/linux-2.6 drm-nouveau-next
> > head:   d2989b534ef6834ebf2425aecc040b894b567c91
> > commit: 01672ef454307bf63e93defb3599399b678ff58b [3/68] drm/nve0/fifo: copy engine context stored in ramfc, not externally
> > 
> > ERROR: trailing statements should be on next line
> > #28: FILE: drivers/gpu/drm/nouveau/core/engine/fifo/nve0.c:146:
> > +       case NVDEV_ENGINE_GR   : addr = 0x0210; break;
> 
> Not false positives.

Yeah, however it does represent a common practice that I feel like
respecting the developers' own choice.

Thanks,
Fengguang