Undefined behavior in Audio::playMusic

[Lord-Kamina]

Around 2009 and 2010; when the fading system was implemented in several leaps and with the Audio system as a whole being heavily refactored at the same time, we went from if (fadeTime) > 0) m_streams.back()->fadein(fadeTime); (see 7db5142)

To m_streams.back()->fadein(fadeTime); in ab6978f.

Then in early 2010, 04c09a2 changed it to m.fadeRate = fadeTime / 48000.0f (which in practice is what getSR() would return.)

Until finally e9e9e09 changed it up and flipped the operands, Leaving us with m->fadeRate = 1.0 / getSR() / fadeTime;

Which, when called from ScreenSing::enter() (or whatever that is called) effectively means we're dividing by 0.0.

@Tronic I doubt this was intentional; can you explain why you flipped the operands back then and how do you think we should fix it?

[Tronic]

I think that m.fadeRate = fadeTime / 48000.0f was simply a bug fixed soon after. You need to divide by duration for correct results (i.e. slower rate with longer time).

Since this is floating-point division, it is not UB. Dividing by zero gets you +inf which actually works out just the way it is supposed to (you get fadeLevel = 1.0 on the first iteration).

[OznOg]

Division by zero is UB in language itself, it may just be optionally supported. (see https://stackoverflow.com/questions/42926763/the-behaviour-of-floating-point-division-by-zero ). That said, maybe platforms we are working on are all supporting it...

[Tronic]

The warning was removed in Clang 9 because they, like every damn compiler and CPU do support float division by zero as per Annex F. If this is a problem with an older Clang release, I suggest disabling the warning explicitly.

[OznOg]

ok, lets go with that :) Do we add a comment to say it is on purpose (for future devs?)

[Baklap4]

Do we add a comment to say it is on purpose (for future devs?)

Yes please!

[Baklap4]

Todo

• [ ] Add a comment to say this division by 0 is intentional.
• [ ] Optional: Disable the warning explicitly (in line).