Messages in this thread | | | Date | Mon, 07 Jun 2004 15:14:06 +0200 | From | Takashi Iwai <> | Subject | Re: [RFC] ASLA design, depth of code review and lack thereof |
| |
At Sat, 5 Jun 2004 01:04:10 +0100, viro@parcelfarce.linux.theplanet.co.uk wrote: > > On Fri, Jun 04, 2004 at 04:29:20PM -0700, Linus Torvalds wrote: > > > > > > On Sat, 5 Jun 2004 viro@parcelfarce.linux.theplanet.co.uk wrote: > > > > > > > > > case SNDRV_PCM_FORMAT_FLOAT_BE: > > > { > > > union { > > > float f; > > > u_int32_t i; > > > } u; > > > u.f = 0.0; > > > #ifdef SNDRV_LITTLE_ENDIAN > > > return bswap_32(u.i); > > > #else > > > return u.i; > > > #endif > > > > So what I wonder about is why anybody does something like this in the > > first place? > > > > Any IEEE format architecture will make 0.0 be all-zeroes, last I saw. In > > fact, any architecture (IEEE or not) where that isn't true will have > > serious problems with floating point values in bss (hint: the bss isn't > > initialzed to 0.0, it's initialized to the bit pattern 0). > > > > So what the above boils dow to is a very very strange way of writing > > > > return 0; > > > > and it has absolutely _zero_ to do with "little-endian" or anything else > > for that matter. > > While we are at it, the function in question > (sound/core/pcm_misc.::snd_pcm_format_silence_64()) > is a plain and simple array in disguise - it should've been > > u_int64_t snd_pcm_format_silence_64(snd_pcm_format_t format) > { > unsigned index = snd_enum_to_int(format); /* just a cast, hopefully */ > return index < SNDRV_PCM_FORMAT_LAST ? filler[index] : -EINVAL; > } > > And -EINVAL here, BTW, is a broken interface - none of its callers are > checking for -EINVAL and that's a hell of an odd error value when normal > values can be all over the place in u64 anyway.
Agreed. (Long time ago, we had supported not so many formats like now, so switch() was ok at that time... :)
> Looking at those callers, I'd say that this guy should be > > unsigned char * find_filler(snd_pcm_format_t format) > > and return a pointer to a (8-element) row in array of patterns. Because > callers end up truncating the result and then filling a large area with > repeated copies. All we get from use of u_int64_t is extra PITA with > endianness - memcpy from 1/2/4/8 element array is no less efficient than > assignment from u8/u16/u32/u64.
Is it true? If gcc really optmizes well like this, yes, surely we can use memcpy for simplicity.
-- Takashi Iwai <tiwai@suse.de> ALSA Developer - www.alsa-project.org - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |