From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: patch for hammerfall driver Date: Mon, 03 Feb 2003 13:40:52 +0100 Sender: alsa-devel-admin@lists.sourceforge.net Message-ID: References: <200302022137.h12LbkD30723@linuxaudiosystems.com> <3E3E6079.2050905@monmouth.com> Mime-Version: 1.0 (generated by SEMI 1.14.4 - "Hosorogi") Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: <3E3E6079.2050905@monmouth.com> Errors-To: alsa-devel-admin@lists.sourceforge.net List-Help: List-Post: List-Subscribe: , List-Unsubscribe: , List-Archive: To: "John S. Denker" Cc: Paul Davis , alsa-devel@lists.sourceforge.net List-Id: alsa-devel@alsa-project.org At Mon, 03 Feb 2003 07:28:41 -0500, John S. Denker 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