MSP430 Driver Library: possible bug in ADC12_enableInterrupt ()

Piergiuseppe Tundo said:

# define ADC12OVIE0 (ADC12OVIE * 0x10000)

It should better be ( (unsigned long)ADC12OVIE << 16) as this is not only better readable, but depending on how it is interpreted by the compiler, it is the more typesafe and better optimizable notation.

Some compilers make a multiplication when usign the * operator, even if if could be done with a faster shift or by just moving the original value into the high word instead of the low word, like in this case. And the (unsigned long) is necessary since ADC12OVIE is not a long value by itself, so a shift by 16 could be truncated.

I agree about readability but not about computational issue, as this should be done at compile time.

Regards,

P.

Piergiuseppe Tundo said:

I agree about readability but not about computational issue, as this should be done at compile time.

In this specific case, yes. Since both are constants. But it's good style to do things 'right' always, so one doesn't forget it if it matters :)
Also, you're right with 'should'. It is not really required that a compiler will do this at compile time. A poor implementation may just produce runtime code, and it wouldn't affect the program outcome.
And if the symbol is no constant but an extern symbol (which it isn't or at least shouldn't in this case), there is no way but to do it at runtime.

It's even possible that the original 0x10000 without trailing L might be treated as a signed int constant, resulting in a *0 operation and a zero result. If 'int' defaults to 'short int', the compiler may parse constants without L modifier as short ints and issue a warning. Or not even that.