Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai <tiwai@suse.de>]

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