[alsa-devel] Kill snd_assert()
Hi,
as some people already mentioned, we have a pretty ugly macro, snd_assert(). Actually, this macro is sometimes useful, but the implementation including the code flow in arguments is really bad.
I tried to kill this, either the following way:
- replace snd_assert(cond, flow...); with if (snd_BUG_ON(!cond)) flow...;
- use WARN_ON() if the check is needed essentially regardless of CONFIG_SND_DBEBUG value
- remove snd_assert() for obviously unneeded code-paths
The new snd_BUG_ON() macro takes the condition, and works like WARN_ON() if CONFIG_SND_DEBUG=y: prints the warning with stack trace, and returns the evaluated value. When CONFIG_SND_DEBUG=n, the macro returns always zero, so that the compiler will optimize out the unreached flows.
Since snd_BUG_ON() takes the condition of a bug, it's a negative condition against snd_assert(). It follows BUG_ON() and WARN_ON(), and easier to understand, IMO.
The patches are found on topic/remove-assert branch of my git tree:
git://git.kernel.org/pub/scm/linux/kernel/tiwai/sound-2.6.git
For gitweb, http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=t...
I haven't merged this topic yet. If you have a big objection or a better implementation idea, please let me know.
Takashi
On Mon, 11 Aug 2008, Takashi Iwai wrote:
Hi,
as some people already mentioned, we have a pretty ugly macro, snd_assert(). Actually, this macro is sometimes useful, but the implementation including the code flow in arguments is really bad.
I tried to kill this, either the following way:
- replace snd_assert(cond, flow...); with if (snd_BUG_ON(!cond)) flow...;
For my eyes, many uses of snd_assert with direct 'return VALUE' are more "sexy", easy understandable and smaller. But, as mentioned, when goto is used to change the code flow, we end up with a compiler warning. It should be eliminated.
I have no strong objections to accept this change as you proposed. Thank you for your work.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Mon, 11 Aug 2008 10:52:23 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 11 Aug 2008, Takashi Iwai wrote:
Hi,
as some people already mentioned, we have a pretty ugly macro, snd_assert(). Actually, this macro is sometimes useful, but the implementation including the code flow in arguments is really bad.
I tried to kill this, either the following way:
- replace snd_assert(cond, flow...); with if (snd_BUG_ON(!cond)) flow...;
For my eyes, many uses of snd_assert with direct 'return VALUE' are more "sexy", easy understandable and smaller.
There are pros and cons for this, too. One merit of snd_assert() is its shortness, thus easier to add. But, mining the code flow is dangerous especially if you want to add a paired operation like locks or move the assert to another place. With the explicit code flow, you could follow more easily.
But, as mentioned, when goto is used to change the code flow, we end up with a compiler warning. It should be eliminated.
Right.
I have no strong objections to accept this change as you proposed. Thank you for your work.
OK, good to hear.
thanks,
Takashi
At Mon, 11 Aug 2008 10:41:37 +0200, I wrote:
Hi,
as some people already mentioned, we have a pretty ugly macro, snd_assert(). Actually, this macro is sometimes useful, but the implementation including the code flow in arguments is really bad.
I tried to kill this, either the following way:
replace snd_assert(cond, flow...); with if (snd_BUG_ON(!cond)) flow...;
use WARN_ON() if the check is needed essentially regardless of CONFIG_SND_DBEBUG value
remove snd_assert() for obviously unneeded code-paths
The new snd_BUG_ON() macro takes the condition, and works like WARN_ON() if CONFIG_SND_DEBUG=y: prints the warning with stack trace, and returns the evaluated value. When CONFIG_SND_DEBUG=n, the macro returns always zero, so that the compiler will optimize out the unreached flows.
Since snd_BUG_ON() takes the condition of a bug, it's a negative condition against snd_assert(). It follows BUG_ON() and WARN_ON(), and easier to understand, IMO.
The patches are found on topic/remove-assert branch of my git tree:
git://git.kernel.org/pub/scm/linux/kernel/tiwai/sound-2.6.git
Oops, the correct URL is: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
Also, the corresponding alsa-driver build tree is found on topic/remove-assert branch of alsa-driver-build.git below: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/alsa-driver-build.git
http://git.kernel.org/?p=linux/kernel/git/tiwai/alsa-driver-build.git;a=shor...
Takashi
participants (2)
-
Jaroslav Kysela -
Takashi Iwai