On Mon, 2010-12-20 at 17:47 +0100, Takashi Iwai wrote:
At Mon, 20 Dec 2010 16:27:49 +0000, Dimitris Papastamos wrote:
On Mon, 2010-12-20 at 17:05 +0100, Takashi Iwai wrote:
+static inline bool set_cache_val(void *base, unsigned int idx,
unsigned int val, unsigned int word_size)+{
- if (word_size == 1) {
u8 *cache = base;if (cache[idx] == val)return true;cache[idx] = val;- } else {
u16 *cache = base;if (cache[idx] == val)return true;cache[idx] = val;- }
- return false;
+}
If word_size is anything other than 1 byte, the above else will try to handle it and assume it is 16 bits. I'd expect for an explicit check for word_size == 2. A switch statement would perhaps be preferred for legibility. It'd perhaps be wise to simply die via BUG() or similar if an unsupported word size was passed in.
Yes, this would be safer. I didn't put it since I wasn't sure whether BUG() content is also expanded at each caller. If yes, it would bloat. (Alternatively we may use snd_BUG_ON() -- it's built in only when CONFIG_SND_DEBUG is set.)
Anyway, such a check should have been done rather at initialization.
I'd expect macro expansion to happen before inline function expansion.
I'll send in a patch to perform this check at init time once yours has been merged.
Thanks, Dimitrios