This can happen in release builds anytime. It won’t be fixed with a debug assertion!
void paint (Graphics& g) override
{
float v=(float)1.17549e-038;
v-=0.1f;
v+=0.1f;
RectangleList<float> rl;
rl.add(-1.f/v,10.f,10.f,1.f/v);
g.fillRectList(rl);
}
We added an extra assertion in the code to warn users when they are supplying non-finite values to rectangle lists. However, we haven't added any extra guards. To me it is not clear which code path should be taken when RectangleList comes across an invalid rectangle which the user shouldn't have supplied in the first place. Besides, any extra checks in RectangleList will incur a significant performance penalty anyway and RectangleLists are used in performance sensitive places throughout the JUCE code. To me, the only sensible solution is to ensure that the invalid rectangle is caught at a higher level - which is in your spectrum analyser code.
Thanks, i already did this. I can live with that!
But something in my head is says this isn't a perfect solution, because its nature of floats that they can swap to INF and NAN states, without any exception.
What about adding two differents methods to RectangleList, one could be
rl.addUnchecked() // for internal use
rl.add() // for anybody else
We discussed this, but if you give the RectangleList garbage coordinates, it's not clear what you expect it to do with them.
That's why we think the correct way to handle this is to say that if you must give it finite rectangles, and if you don't then you'll get undefined behaviour.
>We discussed this, but if you give the RectangleList garbage coordinates, it's not clear what you expect it to do with them.
Well anything, but not to crash the entire application ;)
This is all under the special aspect how float-values behave, under different circumstances or different architectures, this might not crash, the developer has no chance to see the problem. And we all make mistakes!
Yes we can call it undefined behavior, but if you look how many classes RectangleList are using, and in the end how many other classes trust RectangleList, and classes who trust this classes, for my taste that's much too undefined behavior, did you check them all?
I don't like undefined behavior, sometimes unavoidable of cause, bit it should not used as a "tool".
And since when this is an undefined behavior, since your last commit!
If this software would be security relevant, it would be a nightmare. Crashing is perfect! But with an defined exception, and not with a random access to memory.
But if its crash, its better with to crash with a defined exception, and not with a random memory access, in terms of security.
Sorry, enough moralizing... ;-)
Yes we can call it undefined behavior, but if you look how many classes RectangleList are using, and in the end how many other classes trust RectangleList, and classes who trust this classes, for my taste that's much too undefined behavior, did you check them all?
In a sense, yes, we've been checking them all for years. We (and hundreds other people) are running countless apps, plugins and tests that hammer all the code-paths that might use these methods. The assertions have always been there to catch these things, so we'd have found out about any problems by now.
I'm happy to add more assertions to try to catch these things early and to add comments reminding people to be careful, but TBH if people feed broken numbers into these functions and hit an assertion, I was kind of expecting that they'd consider it their own mistake rather than something the library is doing wrong!
If my attitude seems a bit harsh, it's pretty mainstream in the C++ world to take this kind of stance - the whole idea of "undefined behaviour" is there so that library code doesn't have to be bloated and slowed-down by a million checks for every possible mistake that a user could make.
My point of view is, that a mis-aligned float supplied to an UI-display method in a release build, should not crash (CRASH! bad access!) an application.
Nature of Float-Values, UI, Release-build, Crash, the combination off all 5 is it!
In terms of performance, i think we should consulate a profiler first.
