false positive in checkpatch.pl (complex macro values)

false positive in checkpatch.pl (complex macro values)

[Mike Frysinger]

in some code that does like:
#define foo { a, b, c, \
    d, e, f, g }
...
int boo[] = foo;
...

checkpatch.pl throws a fit:
ERROR: Macros with complex values should be enclosed in parenthesis
#10: FILE: ...
+#define foo {a, b, c, d}

perhaps the check should also allow {...} ?  or ignore lists like this ...
-mike

Re: false positive in checkpatch.pl (complex macro values)

[Andy Whitcroft]

Mike Frysinger wrote:
> in some code that does like:
> #define foo { a, b, c, \
>     d, e, f, g }
> ...
> int boo[] = foo;
> ...
> 
> checkpatch.pl throws a fit:
> ERROR: Macros with complex values should be enclosed in parenthesis
> #10: FILE: ...
> +#define foo {a, b, c, d}
> 
> perhaps the check should also allow {...} ?  or ignore lists like this ...
> -mike

Ok, we can add that to the check.  Next update will allow that.

Thanks for the report.

-apw

Re: false positive in checkpatch.pl (complex macro values)

[SL Baur]

On 8/24/07, Andy Whitcroft <apw@shadowen.org> wrote:
> Mike Frysinger wrote:
> > in some code that does like:
> > #define foo { a, b, c, \
> >     d, e, f, g }
> > ...
> > int boo[] = foo;
> > ...
> >
> > checkpatch.pl throws a fit:
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #10: FILE: ...
> > +#define foo {a, b, c, d}
> >
> > perhaps the check should also allow {...} ?  or ignore lists like this ...
> > -mike
>
> Ok, we can add that to the check.  Next update will allow that.
>
> Thanks for the report.

I sent a reply accidentally only to Mike and not the list.  I think the
error message is wrong.  That is really ugly code.  Linux Kernel code
believes in C not preprocessor tricks, so why would you need this?

Who uses code like this, by the way?

-sb

Re: false positive in checkpatch.pl (complex macro values)

[SL Baur]

On 8/24/07, SL Baur <steve@xemacs.org> wrote:

> I think the
> error message is wrong.

I mean the error message is badly worded.  That's bad C and the
macro needs deletion a lot more than it needs an extra set of parens.

Been chasing a heisen bug too long.  Need sleep.  Sorry.

-sb

Re: false positive in checkpatch.pl (complex macro values)

[Mike Frysinger]

On 8/24/07, SL Baur <steve@xemacs.org> wrote:
> On 8/24/07, Andy Whitcroft <apw@shadowen.org> wrote:
> > Mike Frysinger wrote:
> > > in some code that does like:
> > > #define foo { a, b, c, \
> > >     d, e, f, g }
> > > ...
> > > int boo[] = foo;
> > > ...
> > >
> > > checkpatch.pl throws a fit:
> > > ERROR: Macros with complex values should be enclosed in parenthesis
> > > #10: FILE: ...
> > > +#define foo {a, b, c, d}
> > >
> > > perhaps the check should also allow {...} ?  or ignore lists like this ...
> >
> > Ok, we can add that to the check.  Next update will allow that.
> >
> > Thanks for the report.
>
> I sent a reply accidentally only to Mike and not the list.  I think the
> error message is wrong.  That is really ugly code.  Linux Kernel code
> believes in C not preprocessor tricks, so why would you need this?
>
> Who uses code like this, by the way?

the way we came across it was in code that hasnt been pushed to
mainline yet ... we run the code on our tree

however, there are plenty of cases in the tree right now ... a quick
grep picks out sound/pci/ice1712/aureon.h as the first offender

$ diff -Nu /dev/null ./sound/pci/ice1712/aureon.h | perl
./scripts/checkpatch.pl -
ERROR: Macros with complex values should be enclosed in parenthesis
#31: FILE: sound/pci/ice1712/aureon.h:28:
+#define  AUREON_DEVICE_DESC           "{Terratec,Aureon 5.1 Sky},"\

$ grep AUREON_DEVICE_DESC ./sound/pci/ice1712/aureon.h -A 5
#define  AUREON_DEVICE_DESC            "{Terratec,Aureon 5.1 Sky},"\
                                       "{Terratec,Aureon 7.1 Space},"\
                                       "{Terratec,Aureon 7.1 Universe}," \
                                        "{AudioTrak,Prodigy 7.1}," \
                                        "{AudioTrak,Prodigy 7.1 LT},"\
                                        "{AudioTrak,Prodigy 7.1 XT},"

seems like perfectly valid usage to me
-mike

Re: false positive in checkpatch.pl (complex macro values)

[Josef Sipek]

On Fri, Aug 24, 2007 at 09:24:17AM -0400, Mike Frysinger wrote:
...
> $ grep AUREON_DEVICE_DESC ./sound/pci/ice1712/aureon.h -A 5
> #define  AUREON_DEVICE_DESC            "{Terratec,Aureon 5.1 Sky},"\
>                                        "{Terratec,Aureon 7.1 Space},"\
>                                        "{Terratec,Aureon 7.1 Universe}," \
>                                         "{AudioTrak,Prodigy 7.1}," \
>                                         "{AudioTrak,Prodigy 7.1 LT},"\
>                                         "{AudioTrak,Prodigy 7.1 XT},"

This is a different thing. This is a long string, not a list of elements.
IMO, this one shouldn't have given a warning regardless of whether or not
the original case is valid.

Josef 'Jeff' Sipek.

-- 
If I have trouble installing Linux, something is wrong. Very wrong.
		- Linus Torvalds

Re: false positive in checkpatch.pl (complex macro values)

[Olivier Galibert]

On Fri, Aug 24, 2007 at 05:43:47AM -0700, SL Baur wrote:
> Who uses code like this, by the way?

People who think Posix is an example to follow maybe?  Not sure if it
would go past the maintainers though :-)

# define PTHREAD_MUTEX_INITIALIZER \
  { { 0, 0, 0, 0, 0, { 0 } } }
# ifdef __USE_GNU
#  define PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP \
  { { 0, 0, 0, PTHREAD_MUTEX_RECURSIVE_NP, 0, { 0 } } }
#  define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP \
  { { 0, 0, 0, PTHREAD_MUTEX_ERRORCHECK_NP, 0, { 0 } } }
#  define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP \
  { { 0, 0, 0, PTHREAD_MUTEX_ADAPTIVE_NP, 0, { 0 } } }
# endif

  OG.

Re: false positive in checkpatch.pl (complex macro values)

[Mike Frysinger]

On 8/24/07, Josef Sipek <jsipek@fsl.cs.sunysb.edu> wrote:
> On Fri, Aug 24, 2007 at 09:24:17AM -0400, Mike Frysinger wrote:
> ...
> > $ grep AUREON_DEVICE_DESC ./sound/pci/ice1712/aureon.h -A 5
> > #define  AUREON_DEVICE_DESC            "{Terratec,Aureon 5.1 Sky},"\
> >                                        "{Terratec,Aureon 7.1 Space},"\
> >                                        "{Terratec,Aureon 7.1 Universe}," \
> >                                         "{AudioTrak,Prodigy 7.1}," \
> >                                         "{AudioTrak,Prodigy 7.1 LT},"\
> >                                         "{AudioTrak,Prodigy 7.1 XT},"
>
> This is a different thing. This is a long string, not a list of elements.
> IMO, this one shouldn't have given a warning regardless of whether or not
> the original case is valid.

i did point out that grepping the tree shows plenty of results ... if
this one is not satisfactory, you're free to grep to locate ones that
are

the code we're using is used to initialize a data structure ... you
can find similar things in:
./sound/ppc/tumbler.c
./sound/pci/emu10k1/p16v.c
./sound/pci/hda/patch_realtek.c
./sound/pci/hda/patch_si3054.c
./sound/usb/usbquirks.h
./sound/oss/sscape.c
and then i hit ctrl+c
-mike