Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Wed, Jan 18, 2012 at 11:38:34PM +0530, Pradheep Shrinivasan wrote:
>> > > -#define PMEM_IOCTL_MAGIC 'p'
> > > +#define PMEM_IOCTL_MAGIC ('p')
> >
> > You don't need parenthesis here.  Did checkpatch really complain
> > about this?
>
> Yes the check patch does complain about the parenthesis.
> 
> pradheep@ubuntu:~/linux-next/
> linux-next/drivers/staging/android$ checkpatch android_pmem.h
> android_pmem.h:10: ERROR: trailing whitespace
> android_pmem.h:19: ERROR: Macros with complex values should be enclosed in
> parenthesis

That seems like a bug in checkpatch.pl.  It's hard to imagine less
complex macro than:  #define PMEM_IOCTL_MAGIC 'p'

Perhaps if the check looked for one of these characters: */+-=<>|&

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Andy Whitcroft]

On Wed, Jan 18, 2012 at 6:54 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Jan 18, 2012 at 11:38:34PM +0530, Pradheep Shrinivasan wrote:
>>> > > -#define PMEM_IOCTL_MAGIC 'p'
>> > > +#define PMEM_IOCTL_MAGIC ('p')
>> >
>> > You don't need parenthesis here.  Did checkpatch really complain
>> > about this?
>>
>> Yes the check patch does complain about the parenthesis.
>>
>> pradheep@ubuntu:~/linux-next/
>> linux-next/drivers/staging/android$ checkpatch android_pmem.h
>> android_pmem.h:10: ERROR: trailing whitespace
>> android_pmem.h:19: ERROR: Macros with complex values should be enclosed in
>> parenthesis
>
> That seems like a bug in checkpatch.pl.  It's hard to imagine less
> complex macro than:  #define PMEM_IOCTL_MAGIC 'p'

I can think of no way in which an unprotected character is different
to an unprotected integer constant.  So ...

Does the version here work better for you:

    http://people.canonical.com/~apw/checkpatch/checkpatch-next.pl

-apw

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

It still complains about the following macros where parenthesis are
not needed.

ERROR: Macros with complex values should be enclosed in parenthesis
#156: FILE: staging/android/pmem.c:156:
+#define PMEM_IS_FREE(id, index) !(pmem[id].bitmap[index].allocated)

Let's just make the check look for an operator with a low
precedence.
http://en.wikipedia.org/wiki/Order_of_operations#Programming_languages

Otherwise the submitters are going to change it to:

#define PMEM_IS_FREE(id, index) (!(pmem[id].bitmap[index].allocated))

That has two pairs of unneeded paranthesis and we run the risk of
reprogramming the kernel in lisp, by mistake.

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Andy Whitcroft]

On Fri, Jan 20, 2012 at 11:54 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> It still complains about the following macros where parenthesis are
> not needed.
>
> ERROR: Macros with complex values should be enclosed in parenthesis
> #156: FILE: staging/android/pmem.c:156:
> +#define PMEM_IS_FREE(id, index) !(pmem[id].bitmap[index].allocated)
>
> Let's just make the check look for an operator with a low
> precedence.
> http://en.wikipedia.org/wiki/Order_of_operations#Programming_languages
>
> Otherwise the submitters are going to change it to:
>
> #define PMEM_IS_FREE(id, index) (!(pmem[id].bitmap[index].allocated))
>
> That has two pairs of unneeded paranthesis and we run the risk of
> reprogramming the kernel in lisp, by mistake.

Yep we want to avoid that.  As -> binds tighter than many of the
operators in these bands we can only safely avoid the ()'s if the
operator does not potentially make a pointer, so for now I have added
unary minus and the two unary not operators.  I think I want to see
some examples of other operators before I would be keen to have any
further exceptions.

Anyhow how does this work for you:

    http://people.canonical.com/~apw/checkpatch/checkpatch-next.pl

-apw

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Joe Perches]

On Fri, 2012-01-20 at 14:54 +0300, Dan Carpenter wrote:
> It still complains about the following macros where parenthesis are
> not needed.
> 
> ERROR: Macros with complex values should be enclosed in parenthesis
> #156: FILE: staging/android/pmem.c:156:
> +#define PMEM_IS_FREE(id, index) !(pmem[id].bitmap[index].allocated)
> 
> Let's just make the check look for an operator with a low
> precedence.
> http://en.wikipedia.org/wiki/Order_of_operations#Programming_languages
> 
> Otherwise the submitters are going to change it to:
> 
> #define PMEM_IS_FREE(id, index) (!(pmem[id].bitmap[index].allocated))
> 
> That has two pairs of unneeded paranthesis and we run the risk of
> reprogramming the kernel in lisp, by mistake.

I think the outer parens are necessary.
Imagine PMEM_IS_FREE(foo, bar).another_dereference

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

On Fri, Jan 20, 2012 at 05:29:04AM -0800, Joe Perches wrote:
> > #define PMEM_IS_FREE(id, index) (!(pmem[id].bitmap[index].allocated))
> > 
> > That has two pairs of unneeded paranthesis and we run the risk of
> > reprogramming the kernel in lisp, by mistake.
> 
> I think the outer parens are necessary.
> Imagine PMEM_IS_FREE(foo, bar).another_dereference

That's not going to happen in real life.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Andy Whitcroft]

On Fri, Jan 20, 2012 at 05:29:04AM -0800, Joe Perches wrote:
> On Fri, 2012-01-20 at 14:54 +0300, Dan Carpenter wrote:
> > It still complains about the following macros where parenthesis are
> > not needed.
> > 
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #156: FILE: staging/android/pmem.c:156:
> > +#define PMEM_IS_FREE(id, index) !(pmem[id].bitmap[index].allocated)
> > 
> > Let's just make the check look for an operator with a low
> > precedence.
> > http://en.wikipedia.org/wiki/Order_of_operations#Programming_languages
> > 
> > Otherwise the submitters are going to change it to:
> > 
> > #define PMEM_IS_FREE(id, index) (!(pmem[id].bitmap[index].allocated))
> > 
> > That has two pairs of unneeded paranthesis and we run the risk of
> > reprogramming the kernel in lisp, by mistake.
> 
> I think the outer parens are necessary.
> Imagine PMEM_IS_FREE(foo, bar).another_dereference


I don't believe that makes any sense does it, those are pointer
operations, and passing 0/1 to those is going to make the . very
unhappy.  But that is why I am only proposing to do the three which are
non-sensicle on pointers.

-apw

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Joe Perches]

On Fri, 2012-01-20 at 11:12 +0000, Andy Whitcroft wrote:
> On Wed, Jan 18, 2012 at 6:54 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Wed, Jan 18, 2012 at 11:38:34PM +0530, Pradheep Shrinivasan wrote:
> >>> > > -#define PMEM_IOCTL_MAGIC 'p'
> >> > > +#define PMEM_IOCTL_MAGIC ('p')
> >> > You don't need parenthesis here.  Did checkpatch really complain
> >> > about this?
> >> Yes the check patch does complain about the parenthesis.
> > That seems like a bug in checkpatch.pl.  It's hard to imagine less
> > complex macro than:  #define PMEM_IOCTL_MAGIC 'p'
> I can think of no way in which an unprotected character is different
> to an unprotected integer constant.  So ...
> Does the version here work better for you:
>     http://people.canonical.com/~apw/checkpatch/checkpatch-next.pl

diff here is:

@@ -2838,7 +2849,8 @@
                        if ($dstat ne '' &&
                            $dstat !~ /^(?:$Ident|-?$Constant),$/ &&                    # 10, // foo(),
                            $dstat !~ /^(?:$Ident|-?$Constant);$/ &&                    # foo();
-                           $dstat !~ /^(?:$Ident|-?$Constant)$/ &&                     # 10 // foo()
+                           $dstat !~ /^[!~-]?(?:$Ident|$Constant)$/ &&         # 10 // foo() // !foo // ~foo // -foo
+                           $dstat !~ /^'X'$/ &&                                        # character constants
                            $dstat !~ /$exceptions/ &&
                            $dstat !~ /^\.$Ident\s*=/ &&                                # .foo =
                            $dstat !~ /^do\s*$Constant\s*while\s*$Constant;?$/ &&       # do {...} while (...); // do {...} while (...)

I think the character change test is fine but
the !~- addition/change is suspect.

!~- are precedence level 3 operators
and can be impacted by things like ++
and function calls.

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On Fri, Jan 20, 2012 at 05:15:15AM -0800, Joe Perches wrote:
> @@ -2838,7 +2849,8 @@
>                         if ($dstat ne '' &&
>                             $dstat !~ /^(?:$Ident|-?$Constant),$/ &&                    # 10, // foo(),
>                             $dstat !~ /^(?:$Ident|-?$Constant);$/ &&                    # foo();
> -                           $dstat !~ /^(?:$Ident|-?$Constant)$/ &&                     # 10 // foo()
> +                           $dstat !~ /^[!~-]?(?:$Ident|$Constant)$/ &&         # 10 // foo() // !foo // ~foo // -foo
> +                           $dstat !~ /^'X'$/ &&                                        # character constants
>                             $dstat !~ /$exceptions/ &&
>                             $dstat !~ /^\.$Ident\s*=/ &&                                # .foo =
>                             $dstat !~ /^do\s*$Constant\s*while\s*$Constant;?$/ &&       # do {...} while (...); // do {...} while (...)
> 
> I think the character change test is fine but
> the !~- addition/change is suspect.
> 
> !~- are precedence level 3 operators

Level 2?

> and can be impacted by things like ++

They work like you would expect:

#define NOT_FOO !foo
	++NOT_FOO <-- compile error.
	NOT_FOO++ <-- works.

Anyway if people are really writing macros which are used this way,
then that's something which is outside of the scope of checkpatch.pl
to fix.

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl

[Joe Perches]

On Fri, 2012-01-20 at 16:46 +0300, Dan Carpenter wrote:
> On Fri, Jan 20, 2012 at 05:15:15AM -0800, Joe Perches wrote:
> > @@ -2838,7 +2849,8 @@
> >                         if ($dstat ne '' &&
> >                             $dstat !~ /^(?:$Ident|-?$Constant),$/ &&                    # 10, // foo(),
> >                             $dstat !~ /^(?:$Ident|-?$Constant);$/ &&                    # foo();
> > -                           $dstat !~ /^(?:$Ident|-?$Constant)$/ &&                     # 10 // foo()
> > +                           $dstat !~ /^[!~-]?(?:$Ident|$Constant)$/ &&         # 10 // foo() // !foo // ~foo // -foo
> > +                           $dstat !~ /^'X'$/ &&                                        # character constants
> >                             $dstat !~ /$exceptions/ &&
> >                             $dstat !~ /^\.$Ident\s*=/ &&                                # .foo =
> >                             $dstat !~ /^do\s*$Constant\s*while\s*$Constant;?$/ &&       # do {...} while (...); // do {...} while (...)
> > I think the character change test is fine but
> > the !~- addition/change is suspect.
> > !~- are precedence level 3 operators
> Level 2?

Table I looked at said 3,
http://publib.boulder.ibm.com/infocenter/macxhelp/v6v81/index.jsp?topic=/com.ibm.vacpp6m.doc/language/ref/clrc05preeval.htm
but 1 is c++ only, so you're right
c level 2.

> Anyway if people are really writing macros which are used this way,
> then that's something which is outside of the scope of checkpatch.pl
> to fix.

I think there are some convenience macros like:
#define helper(a, b) some->long[a]->dereference.b

but likely anyone that really wants to add a
!~- prefix to those should be mindful anyway.