Re: patch for hammerfall driver

[Takashi Iwai <tiwai@suse.de>]

At Mon, 03 Feb 2003 07:28:41 -0500,
John S. Denker <jsd@monmouth.com> wrote:
> 
> Takashi Iwai wrote:
> 
> > (btw, i used 4 instead of sizeof(int) because the latter might not be
> >  always equal with 32bit (in future) :)
> 
> Certainly assuming sizeof(int)==4 is unwise.
> But using "4" isn't the optimal solution, either.
> 
> It is not a good programming practice to throw
> around magic numbers like "4" without documenting
> where they came from.  In this case it would be
> better to use
>    snd_pcm_format_size(format, nsamples)

well, unfortunately, it's not a constant, so we cannot use it as the
initializer.

i believe, in this case, giving a comment like /* 8192 frames */ would
suffice, because it's obvious that the hardware supports only 32bit
samples.  surely it was my fault, only putting a magic number there.

btw, sizeof() is not a general solution for this, too, because the
buffer size is not always aligned to any values of architecture
(e.g. 24bit/3bytes format).


> At Sun, 2 Feb 2003 16:37:46 -0500,
> Paul Davis wrote:
> 
>  >i don't know how we ended up with such incorrect values for the
>  >period/buffer size limits,
> 
> That is a very important question!
> It is by asking such questions that one grows
> as a programmer.
> 
> To be explicit:  whenever you see a bug, don't just
> ask how to fix this instance of the bug;  ask what
> it would take to make sure no bug of this ilk ever
> occurs again.
> 
> In this case:
>   a) It would help to have some comments in the code
> saying where the constants are coming from, so that
> misconceptions can be more easily spotted.
>   b) It would help to use correctly computed constants
> such as
>    snd_pcm_format_size(format, nsamples)
> rather than magic numbers such as "4".  There are
> far too many magic numbers in the ALSA code at
> the moment.
>   c) When applying the sizeof() operator, don't
> apply it to general-purpose types such as int, but
> rather apply it to an object that is provably
> of the type you care about, in this case something
> like sizeof(s32_le_t).  Now there isn't AFAIK any
> typedef for s32_le_t, but you could define one
> (carefully!).  If you don't have such a typedef,
> don't assume sizeof(int) is synonymous with the
> sizeof(the_thing_you_care_about).

for kernel codes, you can use sizeof(s32).


Takashi


-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com