search

adafruit / Adafruit_BME280_Library

Arduino Library for BME280 sensors
Other
328 stars 301 forks source link

Fix undefined behaviour in read16() #81

Closed yumkam closed 3 years ago

yumkam commented 4 years ago

Expression evaluation order is undefined in C/C++ (except for short-circuits in && || and ?: )

Beware: not even compile-tested!

espHorst commented 4 years ago

I agree, evaluation order is undefined, but | (binary or) is commutative (a|b is the same as b|a), thus operation order is not important. So there should be no need for modification.

yumkam commented 4 years ago

Wrong. It is not a|b (which would be symmetric), it is (a<<8)|b or (b<<8)|a (totally different).

espHorst commented 4 years ago

I agree! The order of calling those functions is important: spixfer(0); and _wire->read(); And the order of calling those functions is not defined. So currently it depends on the compiler if the version without your pull request works or not! Thanks for explaining.

thijstriemstra commented 3 years ago

So currently it depends on the compiler if the version without your pull request works or not!

That doesn't sound like a change we want to make.

khoek commented 3 years ago

@thijstriemstra The point @espHorst was making is that the current code exhibits undefined behaviour (it might swap the MSB and LSB of a read 16 bit number), depending on the C compiler (and version) used. The pull requester proposes to fix it.

thijstriemstra commented 3 years ago

The pull requester proposes to fix it.

I see!

caternuson commented 3 years ago

Closing this. Looks like a potentially interesting bug. But the updates for #96 will likely change this code anyway.