* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
[not found] ` <CANz8tm1imZ3njy_uV0tkq0sXwyB5ZLLJsrp4G1zjcBDnJUrjiw@mail.gmail.com>
@ 2012-01-18 18:54 ` Dan Carpenter
2012-01-20 11:12 ` Andy Whitcroft
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2012-01-18 18:54 UTC (permalink / raw)
To: Pradheep Shrinivasan, Andy Whitcroft; +Cc: greg, devel, swetland, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-18 18:54 ` [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl Dan Carpenter
@ 2012-01-20 11:12 ` Andy Whitcroft
2012-01-20 11:54 ` Dan Carpenter
2012-01-20 13:15 ` Joe Perches
0 siblings, 2 replies; 10+ messages in thread
From: Andy Whitcroft @ 2012-01-20 11:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Pradheep Shrinivasan, greg, devel, swetland, linux-kernel,
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 11:12 ` Andy Whitcroft
@ 2012-01-20 11:54 ` Dan Carpenter
2012-01-20 12:18 ` Andy Whitcroft
2012-01-20 13:29 ` Joe Perches
2012-01-20 13:15 ` Joe Perches
1 sibling, 2 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-01-20 11:54 UTC (permalink / raw)
To: Andy Whitcroft; +Cc: Pradheep Shrinivasan, greg, devel, swetland, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 11:54 ` Dan Carpenter
@ 2012-01-20 12:18 ` Andy Whitcroft
2012-01-20 13:29 ` Joe Perches
1 sibling, 0 replies; 10+ messages in thread
From: Andy Whitcroft @ 2012-01-20 12:18 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Pradheep Shrinivasan, greg, devel, swetland, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 11:12 ` Andy Whitcroft
2012-01-20 11:54 ` Dan Carpenter
@ 2012-01-20 13:15 ` Joe Perches
2012-01-20 13:46 ` Dan Carpenter
1 sibling, 1 reply; 10+ messages in thread
From: Joe Perches @ 2012-01-20 13:15 UTC (permalink / raw)
To: Andy Whitcroft
Cc: Dan Carpenter, linux-kernel, swetland, devel, Pradheep Shrinivasan
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 11:54 ` Dan Carpenter
2012-01-20 12:18 ` Andy Whitcroft
@ 2012-01-20 13:29 ` Joe Perches
2012-01-20 13:50 ` Dan Carpenter
2012-01-20 16:28 ` Andy Whitcroft
1 sibling, 2 replies; 10+ messages in thread
From: Joe Perches @ 2012-01-20 13:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andy Whitcroft, Pradheep Shrinivasan, greg, devel, swetland,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 13:15 ` Joe Perches
@ 2012-01-20 13:46 ` Dan Carpenter
2012-01-20 17:12 ` Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2012-01-20 13:46 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Whitcroft, linux-kernel, swetland, devel, Pradheep Shrinivasan
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 13:29 ` Joe Perches
@ 2012-01-20 13:50 ` Dan Carpenter
2012-01-20 16:28 ` Andy Whitcroft
1 sibling, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-01-20 13:50 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Whitcroft, Pradheep Shrinivasan, greg, devel, swetland,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 13:29 ` Joe Perches
2012-01-20 13:50 ` Dan Carpenter
@ 2012-01-20 16:28 ` Andy Whitcroft
1 sibling, 0 replies; 10+ messages in thread
From: Andy Whitcroft @ 2012-01-20 16:28 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Pradheep Shrinivasan, greg, devel, swetland, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl
2012-01-20 13:46 ` Dan Carpenter
@ 2012-01-20 17:12 ` Joe Perches
0 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2012-01-20 17:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andy Whitcroft, swetland, Pradheep Shrinivasan, linux-kernel, devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-01-20 17:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1326856764-2531-1-git-send-email-pradheep.sh@gmail.com>
[not found] ` <20120118065620.GE3294@mwanda>
[not found] ` <CANz8tm1imZ3njy_uV0tkq0sXwyB5ZLLJsrp4G1zjcBDnJUrjiw@mail.gmail.com>
2012-01-18 18:54 ` [PATCH 1/6] staging:android_pmem.h: Fixes the space and other formating issues pointed out by checkpatch.pl Dan Carpenter
2012-01-20 11:12 ` Andy Whitcroft
2012-01-20 11:54 ` Dan Carpenter
2012-01-20 12:18 ` Andy Whitcroft
2012-01-20 13:29 ` Joe Perches
2012-01-20 13:50 ` Dan Carpenter
2012-01-20 16:28 ` Andy Whitcroft
2012-01-20 13:15 ` Joe Perches
2012-01-20 13:46 ` Dan Carpenter
2012-01-20 17:12 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).