[alsa-devel] [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro
Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's heavily broken (apart it also broke my parser).
The 'break' in the macro belongs to the do-while loop and not to the switch. So it always falls through the all cases till the end.
Remove the do-while from the macro.
Also people should terminate statements with semicolons. So force people to do so by removing the last one in the macro.
I vote for removing that crap completely because it makes the code weird anyway -- you have to specify manually both of freq and bits which is I would expect to be avoided exactly by such a macro.
Signed-off-by: Jiri Slaby jslaby@suse.cz Cc: Andreas Mohr andi@lisas.de Cc: Takashi Iwai tiwai@suse.de --- sound/pci/azt3328.c | 32 +++++++++++++++----------------- 1 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c index 6117595..134f55f 100644 --- a/sound/pci/azt3328.c +++ b/sound/pci/azt3328.c @@ -980,29 +980,27 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec, snd_azf3328_dbgcallenter(); switch (bitrate) { #define AZF_FMT_XLATE(in_freq, out_bits) \ - do { \ case AZF_FREQ_ ## in_freq: \ freq = SOUNDFORMAT_FREQ_ ## out_bits; \ - break; \ - } while (0); - AZF_FMT_XLATE(4000, SUSPECTED_4000) - AZF_FMT_XLATE(4800, SUSPECTED_4800) + break + AZF_FMT_XLATE(4000, SUSPECTED_4000); + AZF_FMT_XLATE(4800, SUSPECTED_4800); /* the AZF3328 names it "5510" for some strange reason: */ - AZF_FMT_XLATE(5512, 5510) - AZF_FMT_XLATE(6620, 6620) - AZF_FMT_XLATE(8000, 8000) - AZF_FMT_XLATE(9600, 9600) - AZF_FMT_XLATE(11025, 11025) - AZF_FMT_XLATE(13240, SUSPECTED_13240) - AZF_FMT_XLATE(16000, 16000) - AZF_FMT_XLATE(22050, 22050) - AZF_FMT_XLATE(32000, 32000) + AZF_FMT_XLATE(5512, 5510); + AZF_FMT_XLATE(6620, 6620); + AZF_FMT_XLATE(8000, 8000); + AZF_FMT_XLATE(9600, 9600); + AZF_FMT_XLATE(11025, 11025); + AZF_FMT_XLATE(13240, SUSPECTED_13240); + AZF_FMT_XLATE(16000, 16000); + AZF_FMT_XLATE(22050, 22050); + AZF_FMT_XLATE(32000, 32000); default: snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate); /* fall-through */ - AZF_FMT_XLATE(44100, 44100) - AZF_FMT_XLATE(48000, 48000) - AZF_FMT_XLATE(66200, SUSPECTED_66200) + AZF_FMT_XLATE(44100, 44100); + AZF_FMT_XLATE(48000, 48000); + AZF_FMT_XLATE(66200, SUSPECTED_66200); #undef AZF_FMT_XLATE } /* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */
At Sat, 22 Jan 2011 14:56:10 +0100, Jiri Slaby wrote:
Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's heavily broken (apart it also broke my parser).
The 'break' in the macro belongs to the do-while loop and not to the switch. So it always falls through the all cases till the end.
Remove the do-while from the macro.
Also people should terminate statements with semicolons. So force people to do so by removing the last one in the macro.
I vote for removing that crap completely because it makes the code weird anyway -- you have to specify manually both of freq and bits which is I would expect to be avoided exactly by such a macro.
I agree. Andreas, please either revert back to the plain code, or use a simple look-up table if you don't want switch/case.
thanks,
Takashi
Cleanly revert to non-macro implementation of snd_azf3328_codec_setfmt(), to fix last-minute functionality breakage induced by following checkpatch.pl recommendations without giving them their due full share of thought ("revolting computer, ensuing PEBKAC").
I would like to thank Jiri Slaby for his very timely (in -rc1 even) and unexpected (uncommon hardware) "recognition of the dangerous situation" due to his very commendable static parser use. :)
Reported-by: Jiri Slaby jslaby@suse.cz Signed-off-by: Andreas Mohr andi@lisas.de --- On Sat, Jan 22, 2011 at 05:26:39PM +0100, Takashi Iwai wrote:
At Sat, 22 Jan 2011 14:56:10 +0100, Jiri Slaby wrote:
I vote for removing that crap completely because it makes the code weird anyway -- you have to specify manually both of freq and bits which is I would expect to be avoided exactly by such a macro.
I agree. Andreas, please either revert back to the plain code, or use a simple look-up table if you don't want switch/case.
And indeed, the currently committed state IS broken since listening to audio is strangely devoid of positive effects. :) Sucks to be taking a larger tried and tested patch series into the holidays and then making minor "improvements" to it before submission.
I decided that an additional lookup table isn't really worth it, thus reverting is probably best.
The checkpatch.pl false positive (which triggered the horrid mistake of additionally enclosing this per-case macro within do-while(0)) was:
ERROR: Macros with multiple statements should be enclosed in a do - while loop #20: FILE: azt3328.c:982: +#define AZF_FMT_XLATE(in_freq, out_bits) \ + case AZF_FREQ_ ## in_freq: \ + freq = SOUNDFORMAT_FREQ_ ## out_bits; \ + break;
It is also quite interesting to note that this rather uncommon language syntax (do...while(0) within switch scope) compiled cleanly without issue (no warnings).
Please note that this clean revert (exclusively created from existing development steps) was compile-tested only and not runtime-tested on -rc2. This is due to existing suspected modvers issues on -rc2 (it did work fine in many previous versions, now I'm getting unresolved symbols issues with my -dirty, extra/ modules, perhaps due to having switched to/enabled CONFIG_KERNEL_LZO, CONFIG_DEBUG_SET_MODULE_RONX) which I was unable to rectify sufficiently easily so far in my limited time.
The clean revert will be followed up by corresponding comments and some cleanup within that function in the mid-future.
Thanks,
Andreas Mohr
diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c index 6117595..573594b 100644 --- a/sound/pci/azt3328.c +++ b/sound/pci/azt3328.c @@ -979,31 +979,25 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec,
snd_azf3328_dbgcallenter(); switch (bitrate) { -#define AZF_FMT_XLATE(in_freq, out_bits) \ - do { \ - case AZF_FREQ_ ## in_freq: \ - freq = SOUNDFORMAT_FREQ_ ## out_bits; \ - break; \ - } while (0); - AZF_FMT_XLATE(4000, SUSPECTED_4000) - AZF_FMT_XLATE(4800, SUSPECTED_4800) - /* the AZF3328 names it "5510" for some strange reason: */ - AZF_FMT_XLATE(5512, 5510) - AZF_FMT_XLATE(6620, 6620) - AZF_FMT_XLATE(8000, 8000) - AZF_FMT_XLATE(9600, 9600) - AZF_FMT_XLATE(11025, 11025) - AZF_FMT_XLATE(13240, SUSPECTED_13240) - AZF_FMT_XLATE(16000, 16000) - AZF_FMT_XLATE(22050, 22050) - AZF_FMT_XLATE(32000, 32000) + case AZF_FREQ_4000: freq = SOUNDFORMAT_FREQ_SUSPECTED_4000; break; + case AZF_FREQ_4800: freq = SOUNDFORMAT_FREQ_SUSPECTED_4800; break; + case AZF_FREQ_5512: + /* the AZF3328 names it "5510" for some strange reason */ + freq = SOUNDFORMAT_FREQ_5510; break; + case AZF_FREQ_6620: freq = SOUNDFORMAT_FREQ_6620; break; + case AZF_FREQ_8000: freq = SOUNDFORMAT_FREQ_8000; break; + case AZF_FREQ_9600: freq = SOUNDFORMAT_FREQ_9600; break; + case AZF_FREQ_11025: freq = SOUNDFORMAT_FREQ_11025; break; + case AZF_FREQ_13240: freq = SOUNDFORMAT_FREQ_SUSPECTED_13240; break; + case AZF_FREQ_16000: freq = SOUNDFORMAT_FREQ_16000; break; + case AZF_FREQ_22050: freq = SOUNDFORMAT_FREQ_22050; break; + case AZF_FREQ_32000: freq = SOUNDFORMAT_FREQ_32000; break; default: snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate); /* fall-through */ - AZF_FMT_XLATE(44100, 44100) - AZF_FMT_XLATE(48000, 48000) - AZF_FMT_XLATE(66200, SUSPECTED_66200) -#undef AZF_FMT_XLATE + case AZF_FREQ_44100: freq = SOUNDFORMAT_FREQ_44100; break; + case AZF_FREQ_48000: freq = SOUNDFORMAT_FREQ_48000; break; + case AZF_FREQ_66200: freq = SOUNDFORMAT_FREQ_SUSPECTED_66200; break; } /* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */ /* val = 0xff09; 17m15.098s (13123,478Hz; -> 12000Hz???) hmm, 13237.2Hz? */
At Tue, 25 Jan 2011 06:46:31 +0100, Andreas Mohr wrote:
Cleanly revert to non-macro implementation of snd_azf3328_codec_setfmt(), to fix last-minute functionality breakage induced by following checkpatch.pl recommendations without giving them their due full share of thought ("revolting computer, ensuing PEBKAC").
I would like to thank Jiri Slaby for his very timely (in -rc1 even) and unexpected (uncommon hardware) "recognition of the dangerous situation" due to his very commendable static parser use. :)
Reported-by: Jiri Slaby jslaby@suse.cz Signed-off-by: Andreas Mohr andi@lisas.de
Applied now. Thanks.
Takashi
On Sat, Jan 22, 2011 at 05:26:39PM +0100, Takashi Iwai wrote:
At Sat, 22 Jan 2011 14:56:10 +0100, Jiri Slaby wrote:
I vote for removing that crap completely because it makes the code weird anyway -- you have to specify manually both of freq and bits which is I would expect to be avoided exactly by such a macro.
I agree. Andreas, please either revert back to the plain code, or use a simple look-up table if you don't want switch/case.
And indeed, the currently committed state IS broken since listening to audio is strangely devoid of positive effects. :) Sucks to be taking a larger tried and tested patch series into the holidays and then making minor "improvements" to it before submission.
I decided that an additional lookup table isn't really worth it, thus reverting is probably best.
The checkpatch.pl false positive (which triggered the horrid mistake of additionally enclosing this per-case macro within do-while(0)) was:
ERROR: Macros with multiple statements should be enclosed in a do - while loop #20: FILE: azt3328.c:982: +#define AZF_FMT_XLATE(in_freq, out_bits) \
case AZF_FREQ_ ## in_freq: \freq = SOUNDFORMAT_FREQ_ ## out_bits; \break;It is also quite interesting to note that this rather uncommon language syntax (do...while(0) within switch scope) compiled cleanly without issue (no warnings).
Please note that this clean revert (exclusively created from existing development steps) was compile-tested only and not runtime-tested on -rc2. This is due to existing suspected modvers issues on -rc2 (it did work fine in many previous versions, now I'm getting unresolved symbols issues with my -dirty, extra/ modules, perhaps due to having switched to/enabled CONFIG_KERNEL_LZO, CONFIG_DEBUG_SET_MODULE_RONX) which I was unable to rectify sufficiently easily so far in my limited time.
The clean revert will be followed up by corresponding comments and some cleanup within that function in the mid-future.
Thanks,
Andreas Mohr
diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c index 6117595..573594b 100644 --- a/sound/pci/azt3328.c +++ b/sound/pci/azt3328.c @@ -979,31 +979,25 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec,
snd_azf3328_dbgcallenter(); switch (bitrate) { -#define AZF_FMT_XLATE(in_freq, out_bits) \
- do { \
case AZF_FREQ_ ## in_freq: \freq = SOUNDFORMAT_FREQ_ ## out_bits; \break; \- } while (0);
- AZF_FMT_XLATE(4000, SUSPECTED_4000)
- AZF_FMT_XLATE(4800, SUSPECTED_4800)
- /* the AZF3328 names it "5510" for some strange reason: */
- AZF_FMT_XLATE(5512, 5510)
- AZF_FMT_XLATE(6620, 6620)
- AZF_FMT_XLATE(8000, 8000)
- AZF_FMT_XLATE(9600, 9600)
- AZF_FMT_XLATE(11025, 11025)
- AZF_FMT_XLATE(13240, SUSPECTED_13240)
- AZF_FMT_XLATE(16000, 16000)
- AZF_FMT_XLATE(22050, 22050)
- AZF_FMT_XLATE(32000, 32000)
- case AZF_FREQ_4000: freq = SOUNDFORMAT_FREQ_SUSPECTED_4000; break;
- case AZF_FREQ_4800: freq = SOUNDFORMAT_FREQ_SUSPECTED_4800; break;
- case AZF_FREQ_5512:
/* the AZF3328 names it "5510" for some strange reason */freq = SOUNDFORMAT_FREQ_5510; break;- case AZF_FREQ_6620: freq = SOUNDFORMAT_FREQ_6620; break;
- case AZF_FREQ_8000: freq = SOUNDFORMAT_FREQ_8000; break;
- case AZF_FREQ_9600: freq = SOUNDFORMAT_FREQ_9600; break;
- case AZF_FREQ_11025: freq = SOUNDFORMAT_FREQ_11025; break;
- case AZF_FREQ_13240: freq = SOUNDFORMAT_FREQ_SUSPECTED_13240; break;
- case AZF_FREQ_16000: freq = SOUNDFORMAT_FREQ_16000; break;
- case AZF_FREQ_22050: freq = SOUNDFORMAT_FREQ_22050; break;
- case AZF_FREQ_32000: freq = SOUNDFORMAT_FREQ_32000; break; default: snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate); /* fall-through */
- AZF_FMT_XLATE(44100, 44100)
- AZF_FMT_XLATE(48000, 48000)
- AZF_FMT_XLATE(66200, SUSPECTED_66200)
-#undef AZF_FMT_XLATE
- case AZF_FREQ_44100: freq = SOUNDFORMAT_FREQ_44100; break;
- case AZF_FREQ_48000: freq = SOUNDFORMAT_FREQ_48000; break;
- case AZF_FREQ_66200: freq = SOUNDFORMAT_FREQ_SUSPECTED_66200; break; } /* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */ /* val = 0xff09; 17m15.098s (13123,478Hz; -> 12000Hz???) hmm, 13237.2Hz? */
On 01/25/2011 06:46 AM, Andreas Mohr wrote:
It is also quite interesting to note that this rather uncommon language syntax (do...while(0) within switch scope) compiled cleanly without issue (no warnings).
Note that the switch block is the same like the others and case and default and very similar to goto labels. You can even do: switch (x) { int abc; { case 5: abc = 7; break; } do { case 3: abc = 5; break; } while (0); while (1) { printf("%d\n", abc); default: abc = 1; } } And check what it will do for x == 3, 5, and others :).
regards,
At Tue, 25 Jan 2011 09:23:41 +0100, Jiri Slaby wrote:
On 01/25/2011 06:46 AM, Andreas Mohr wrote:
It is also quite interesting to note that this rather uncommon language syntax (do...while(0) within switch scope) compiled cleanly without issue (no warnings).
Note that the switch block is the same like the others and case and default and very similar to goto labels. You can even do: switch (x) { int abc; { case 5: abc = 7; break; } do { case 3: abc = 5; break; } while (0); while (1) { printf("%d\n", abc); default: abc = 1; } } And check what it will do for x == 3, 5, and others :).
Oh, don't let him bring to the dark side of C again :)
Takashi
On Sat, Jan 22, 2011 at 02:56:10PM +0100, Jiri Slaby wrote:
Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's heavily broken (apart it also broke my parser). The 'break' in the macro belongs to the do-while loop and not to the switch. So it always falls through the all cases till the end.
Remove the do-while from the macro.
Hmm, there doesn't remain much to say other than "Double ouch. Mea culpa.".
However I'd like to add one bit, that the usual macro-side do ... while(0) loop has been added by me due to a checkpatch.pl warning which called for attention. IOW, it could be argued that checkpatch.pl led people into ""improving"" formely "working" (whatever one may think of it) code. Something to be investigated, methinks.
Also people should terminate statements with semicolons. So force people to do so by removing the last one in the macro.
Oh dammit of course, I'm normally implementing it this way as well. Unfortunately I failed to submit it that way in this particular case.
I vote for removing that crap completely because it makes the code weird anyway -- you have to specify manually both of freq and bits which is I would expect to be avoided exactly by such a macro.
The reason that it broke your parser is certainly a very valid one. Macro syntax is known to be arcane and quite variable between compilers (e.g. I read that the gcc people said that they were the only one who _really_ grokked macro evaluation order and token pasting the way it was meant to be implemented - go figure...).
As such I support a change here. I'll try to come up with a translation table instead, or else probably best just revert this wetware fart.
Thanks,
Andreas Mohr
participants (3)
-
Andreas Mohr -
Jiri Slaby -
Takashi Iwai