[PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

The snd_BUG_ON() macro should always expand its argument even if we're
forcing it to false.  This kills off unused warnings that did not exist
before the snd_assert() -> snd_BUG_ON() conversion.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 include/sound/core.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 35424a9..87de9fc 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -387,7 +387,7 @@ void snd_verbose_printd(const char *file, int line, const char *format, ...)
 
 #define snd_printd(fmt, args...)	/* nothing */
 #define snd_BUG()			/* nothing */
-#define snd_BUG_ON(cond)	({/*(void)(cond);*/ 0;})  /* always false */
+#define snd_BUG_ON(cond)	((cond) && 0)  /* always false */
 
 #endif /* CONFIG_SND_DEBUG */
 
-- 
1.6.0.3

Re: [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Takashi Iwai]

At Thu,  6 Nov 2008 21:05:21 -0500,
Mike Frysinger wrote:
> 
> The snd_BUG_ON() macro should always expand its argument even if we're
> forcing it to false.  This kills off unused warnings that did not exist
> before the snd_assert() -> snd_BUG_ON() conversion.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Andrew already change this differently.  Check with the latest version.
And which warning did you get actually?


thanks,

Takashi

> ---
>  include/sound/core.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/sound/core.h b/include/sound/core.h
> index 35424a9..87de9fc 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -387,7 +387,7 @@ void snd_verbose_printd(const char *file, int line, const char *format, ...)
>  
>  #define snd_printd(fmt, args...)	/* nothing */
>  #define snd_BUG()			/* nothing */
> -#define snd_BUG_ON(cond)	({/*(void)(cond);*/ 0;})  /* always false */
> +#define snd_BUG_ON(cond)	((cond) && 0)  /* always false */
>  
>  #endif /* CONFIG_SND_DEBUG */
>  
> -- 
> 1.6.0.3
> 

Re: [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

On Fri, Nov 7, 2008 at 01:22, Takashi Iwai wrote:
> At Thu,  6 Nov 2008 21:05:21 -0500,
> Mike Frysinger wrote:
>> The snd_BUG_ON() macro should always expand its argument even if we're
>> forcing it to false.  This kills off unused warnings that did not exist
>> before the snd_assert() -> snd_BUG_ON() conversion.
>
> Andrew already change this differently.  Check with the latest version.

what are you defining as "latest version" ?  i'm looking in torvalds
linux-2.6.git.

> And which warning did you get actually?

a few codecs define local vars which only get used in snd_BUG_ON() ...
so if snd_BUG_ON() doesnt utilize cond, gcc whines that the var is
unused.
-mike

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Takashi Iwai]

At Fri, 7 Nov 2008 02:09:04 -0500,
Mike Frysinger wrote:
> 
> On Fri, Nov 7, 2008 at 01:22, Takashi Iwai wrote:
> > At Thu,  6 Nov 2008 21:05:21 -0500,
> > Mike Frysinger wrote:
> >> The snd_BUG_ON() macro should always expand its argument even if we're
> >> forcing it to false.  This kills off unused warnings that did not exist
> >> before the snd_assert() -> snd_BUG_ON() conversion.
> >
> > Andrew already change this differently.  Check with the latest version.
> 
> what are you defining as "latest version" ?  i'm looking in torvalds
> linux-2.6.git.

git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

> > And which warning did you get actually?
> 
> a few codecs define local vars which only get used in snd_BUG_ON() ...
> so if snd_BUG_ON() doesnt utilize cond, gcc whines that the var is
> unused.

I know it.  But I thought all these were removed in the actual codes.
I'd like to see the real warning messages.


thanks,

Takashi

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

On Fri, Nov 7, 2008 at 02:15, Takashi Iwai wrote:
> At Fri, 7 Nov 2008 02:09:04 -0500,
> Mike Frysinger wrote:
>> On Fri, Nov 7, 2008 at 01:22, Takashi Iwai wrote:
>> > At Thu,  6 Nov 2008 21:05:21 -0500,
>> > Mike Frysinger wrote:
>> >> The snd_BUG_ON() macro should always expand its argument even if we're
>> >> forcing it to false.  This kills off unused warnings that did not exist
>> >> before the snd_assert() -> snd_BUG_ON() conversion.
>> >
>> > Andrew already change this differently.  Check with the latest version.
>>
>> what are you defining as "latest version" ?  i'm looking in torvalds
>> linux-2.6.git.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
>
>> > And which warning did you get actually?
>>
>> a few codecs define local vars which only get used in snd_BUG_ON() ...
>> so if snd_BUG_ON() doesnt utilize cond, gcc whines that the var is
>> unused.
>
> I know it.  But I thought all these were removed in the actual codes.
> I'd like to see the real warning messages.

seems the warnings where in codec drivers we havent pushed into
mainline yet.  regardless, i dont think auditing code and changing it
just to avoid (cond) references is the way to go.  it also breaks
valid C code if there were side effects in the (cond) as any other
macro which does not properly utilize every argument exactly once.
-mike

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Takashi Iwai]

At Fri, 7 Nov 2008 02:29:25 -0500,
Mike Frysinger wrote:
> 
> On Fri, Nov 7, 2008 at 02:15, Takashi Iwai wrote:
> > At Fri, 7 Nov 2008 02:09:04 -0500,
> > Mike Frysinger wrote:
> >> On Fri, Nov 7, 2008 at 01:22, Takashi Iwai wrote:
> >> > At Thu,  6 Nov 2008 21:05:21 -0500,
> >> > Mike Frysinger wrote:
> >> >> The snd_BUG_ON() macro should always expand its argument even if we're
> >> >> forcing it to false.  This kills off unused warnings that did not exist
> >> >> before the snd_assert() -> snd_BUG_ON() conversion.
> >> >
> >> > Andrew already change this differently.  Check with the latest version.
> >>
> >> what are you defining as "latest version" ?  i'm looking in torvalds
> >> linux-2.6.git.
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
> >
> >> > And which warning did you get actually?
> >>
> >> a few codecs define local vars which only get used in snd_BUG_ON() ...
> >> so if snd_BUG_ON() doesnt utilize cond, gcc whines that the var is
> >> unused.
> >
> > I know it.  But I thought all these were removed in the actual codes.
> > I'd like to see the real warning messages.
> 
> seems the warnings where in codec drivers we havent pushed into
> mainline yet.  regardless, i dont think auditing code and changing it
> just to avoid (cond) references is the way to go.  it also breaks
> valid C code if there were side effects in the (cond) as any other
> macro which does not properly utilize every argument exactly once.

IIRC, just giving 0 in a macro like your patch resulted in another
warning messages in case it's used plainly like:

	snd_BUG_ON(foo);

That's the reason why { 0; } was used in the first place.  But, this
caused yet another warning messages depending on gcc versions, thus
Andrew fixed it previously.

I think the best way is to change the latest code like:

#define snd_BUG_ON(cond)	__snd_bug_on(0 && (cond))


Takashi

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Takashi Iwai]

At Fri, 7 Nov 2008 02:29:25 -0500,
Mike Frysinger wrote:
> 
> it also breaks
> valid C code if there were side effects in the (cond) as any other
> macro which does not properly utilize every argument exactly once.

BTW, what do you mean this exactly?


thanks,

Takashi

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

On Fri, Nov 7, 2008 at 02:36, Takashi Iwai wrote:
> At Fri, 7 Nov 2008 02:29:25 -0500,
> Mike Frysinger wrote:
>> On Fri, Nov 7, 2008 at 02:15, Takashi Iwai wrote:
>> > At Fri, 7 Nov 2008 02:09:04 -0500,
>> > Mike Frysinger wrote:
>> >> On Fri, Nov 7, 2008 at 01:22, Takashi Iwai wrote:
>> >> > At Thu,  6 Nov 2008 21:05:21 -0500,
>> >> > Mike Frysinger wrote:
>> >> >> The snd_BUG_ON() macro should always expand its argument even if we're
>> >> >> forcing it to false.  This kills off unused warnings that did not exist
>> >> >> before the snd_assert() -> snd_BUG_ON() conversion.
>> >> >
>> >> > Andrew already change this differently.  Check with the latest version.
>> >>
>> >> what are you defining as "latest version" ?  i'm looking in torvalds
>> >> linux-2.6.git.
>> >
>> > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
>> >
>> >> > And which warning did you get actually?
>> >>
>> >> a few codecs define local vars which only get used in snd_BUG_ON() ...
>> >> so if snd_BUG_ON() doesnt utilize cond, gcc whines that the var is
>> >> unused.
>> >
>> > I know it.  But I thought all these were removed in the actual codes.
>> > I'd like to see the real warning messages.
>>
>> seems the warnings where in codec drivers we havent pushed into
>> mainline yet.  regardless, i dont think auditing code and changing it
>> just to avoid (cond) references is the way to go.  it also breaks
>> valid C code if there were side effects in the (cond) as any other
>> macro which does not properly utilize every argument exactly once.
>
> IIRC, just giving 0 in a macro like your patch resulted in another
> warning messages in case it's used plainly like:
>
>        snd_BUG_ON(foo);
>
> That's the reason why { 0; } was used in the first place.  But, this
> caused yet another warning messages depending on gcc versions, thus
> Andrew fixed it previously.
>
> I think the best way is to change the latest code like:
>
> #define snd_BUG_ON(cond)        __snd_bug_on(0 && (cond))

that should work i think ...
-mike

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

On Fri, Nov 7, 2008 at 02:38, Takashi Iwai wrote:
> At Fri, 7 Nov 2008 02:29:25 -0500, Mike Frysinger wrote:
>> it also breaks
>> valid C code if there were side effects in the (cond) as any other
>> macro which does not properly utilize every argument exactly once.
>
> BTW, what do you mean this exactly?

any potent statement.  such as assignment or pre/post increment/decrement or ...
-mike

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Takashi Iwai]

At Fri, 7 Nov 2008 02:57:40 -0500,
Mike Frysinger wrote:
> 
> On Fri, Nov 7, 2008 at 02:38, Takashi Iwai wrote:
> > At Fri, 7 Nov 2008 02:29:25 -0500, Mike Frysinger wrote:
> >> it also breaks
> >> valid C code if there were side effects in the (cond) as any other
> >> macro which does not properly utilize every argument exactly once.
> >
> > BTW, what do you mean this exactly?
> 
> any potent statement.  such as assignment or pre/post increment/decrement or ...

Well, in that case, such a code itself is buggy :)


Takashi

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

On Fri, Nov 7, 2008 at 03:03, Takashi Iwai wrote:
> At Fri, 7 Nov 2008 02:57:40 -0500, Mike Frysinger wrote:
>> On Fri, Nov 7, 2008 at 02:38, Takashi Iwai wrote:
>> > At Fri, 7 Nov 2008 02:29:25 -0500, Mike Frysinger wrote:
>> >> it also breaks
>> >> valid C code if there were side effects in the (cond) as any other
>> >> macro which does not properly utilize every argument exactly once.
>> >
>> > BTW, what do you mean this exactly?
>>
>> any potent statement.  such as assignment or pre/post increment/decrement or ...
>
> Well, in that case, such a code itself is buggy :)

i'm not advocating doing this sort of thing, i'm saying that
functions/macros should be written correctly so as to not break
standard C behavior.  a guy developing a codec driver could waste a
lot of time because of this sort of thing.
-mike

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Takashi Iwai]

At Fri, 7 Nov 2008 03:05:56 -0500,
Mike Frysinger wrote:
> 
> On Fri, Nov 7, 2008 at 03:03, Takashi Iwai wrote:
> > At Fri, 7 Nov 2008 02:57:40 -0500, Mike Frysinger wrote:
> >> On Fri, Nov 7, 2008 at 02:38, Takashi Iwai wrote:
> >> > At Fri, 7 Nov 2008 02:29:25 -0500, Mike Frysinger wrote:
> >> >> it also breaks
> >> >> valid C code if there were side effects in the (cond) as any other
> >> >> macro which does not properly utilize every argument exactly once.
> >> >
> >> > BTW, what do you mean this exactly?
> >>
> >> any potent statement.  such as assignment or pre/post increment/decrement or ...
> >
> > Well, in that case, such a code itself is buggy :)
> 
> i'm not advocating doing this sort of thing, i'm saying that
> functions/macros should be written correctly so as to not break
> standard C behavior.  a guy developing a codec driver could waste a
> lot of time because of this sort of thing.

Well, no, it's a clear bug of the driver.

A macro that ignores arguments is normal.  Or do you think assert()
isn't a part of "standard" C ? :)


Takashi

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

On Fri, Nov 7, 2008 at 03:22, Takashi Iwai wrote:
> At Fri, 7 Nov 2008 03:05:56 -0500, Mike Frysinger wrote:
>> On Fri, Nov 7, 2008 at 03:03, Takashi Iwai wrote:
>> > At Fri, 7 Nov 2008 02:57:40 -0500, Mike Frysinger wrote:
>> >> On Fri, Nov 7, 2008 at 02:38, Takashi Iwai wrote:
>> >> > At Fri, 7 Nov 2008 02:29:25 -0500, Mike Frysinger wrote:
>> >> >> it also breaks
>> >> >> valid C code if there were side effects in the (cond) as any other
>> >> >> macro which does not properly utilize every argument exactly once.
>> >> >
>> >> > BTW, what do you mean this exactly?
>> >>
>> >> any potent statement.  such as assignment or pre/post increment/decrement or ...
>> >
>> > Well, in that case, such a code itself is buggy :)
>>
>> i'm not advocating doing this sort of thing, i'm saying that
>> functions/macros should be written correctly so as to not break
>> standard C behavior.  a guy developing a codec driver could waste a
>> lot of time because of this sort of thing.
>
> Well, no, it's a clear bug of the driver.
>
> A macro that ignores arguments is normal.  Or do you think assert()
> isn't a part of "standard" C ? :)

we arent talking about assert() here nor are we talking about assert()
behavior, but i would say it was a poor decision.  the fact that it's
called snd_BUG_ON() instead of snd_WARN_ON() is also a bit broken imo.
 BUG() kills the kernel while WARN() complains, and snd_BUG_ON() is
clearly in the latter category.

that said, you could just define snd_BUG_ON() in terms of WARN_ON()
all the time:
#ifdef CONFIG_SND_DEBUG
# define SND_DEBUG 1
#else
# define SND_DEBUG 0
#endif
#define snd_BUG() WARN(SND_DEBUG, "BUG?\n")
#define snd_BUG_ON(cond) WARN(SND_DEBUG && (cond), "BUG? (%s)\n",
__stringify(cond))
-mike

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Takashi Iwai]

At Fri, 7 Nov 2008 04:30:43 -0500,
Mike Frysinger wrote:
> 
> On Fri, Nov 7, 2008 at 03:22, Takashi Iwai wrote:
> > At Fri, 7 Nov 2008 03:05:56 -0500, Mike Frysinger wrote:
> >> On Fri, Nov 7, 2008 at 03:03, Takashi Iwai wrote:
> >> > At Fri, 7 Nov 2008 02:57:40 -0500, Mike Frysinger wrote:
> >> >> On Fri, Nov 7, 2008 at 02:38, Takashi Iwai wrote:
> >> >> > At Fri, 7 Nov 2008 02:29:25 -0500, Mike Frysinger wrote:
> >> >> >> it also breaks
> >> >> >> valid C code if there were side effects in the (cond) as any other
> >> >> >> macro which does not properly utilize every argument exactly once.
> >> >> >
> >> >> > BTW, what do you mean this exactly?
> >> >>
> >> >> any potent statement.  such as assignment or pre/post increment/decrement or ...
> >> >
> >> > Well, in that case, such a code itself is buggy :)
> >>
> >> i'm not advocating doing this sort of thing, i'm saying that
> >> functions/macros should be written correctly so as to not break
> >> standard C behavior.  a guy developing a codec driver could waste a
> >> lot of time because of this sort of thing.
> >
> > Well, no, it's a clear bug of the driver.
> >
> > A macro that ignores arguments is normal.  Or do you think assert()
> > isn't a part of "standard" C ? :)
> 
> we arent talking about assert() here nor are we talking about assert()
> behavior, but i would say it was a poor decision.  the fact that it's
> called snd_BUG_ON() instead of snd_WARN_ON() is also a bit broken imo.
>  BUG() kills the kernel while WARN() complains, and snd_BUG_ON() is
> clearly in the latter category.

Right, that's a bit confusing.  It came because we had already
snd_BUG() macro.  I took snd_BUG_ON() from the analogy of snd_BUG().

> that said, you could just define snd_BUG_ON() in terms of WARN_ON()
> all the time:

Hm, this looks a good alternative, too.
Though, this was already fixed on my git tree in another way...


Takashi

> #ifdef CONFIG_SND_DEBUG
> # define SND_DEBUG 1
> #else
> # define SND_DEBUG 0
> #endif
> #define snd_BUG() WARN(SND_DEBUG, "BUG?\n")
> #define snd_BUG_ON(cond) WARN(SND_DEBUG && (cond), "BUG? (%s)\n",
> __stringify(cond))
> -mike
> 

Re: [alsa-devel] [PATCH] ALSA: have snd_BUG_ON() always refer to arguments

[Mike Frysinger]

On Fri, Nov 7, 2008 at 04:56, Takashi Iwai wrote:
> At Fri, 7 Nov 2008 04:30:43 -0500, Mike Frysinger wrote:
>> we arent talking about assert() here nor are we talking about assert()
>> behavior, but i would say it was a poor decision.  the fact that it's
>> called snd_BUG_ON() instead of snd_WARN_ON() is also a bit broken imo.
>>  BUG() kills the kernel while WARN() complains, and snd_BUG_ON() is
>> clearly in the latter category.
>
> Right, that's a bit confusing.  It came because we had already
> snd_BUG() macro.  I took snd_BUG_ON() from the analogy of snd_BUG().
>
>> that said, you could just define snd_BUG_ON() in terms of WARN_ON()
>> all the time:
>
> Hm, this looks a good alternative, too.
> Though, this was already fixed on my git tree in another way...

np.  if it's fixed now, i'm happy.  maybe keep in mind my suggestion
if someone else complains about the current code not working for them
:).

thanks for your help.
-mike