* 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: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: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 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 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: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