On Fri, 2008-06-27 at 12:41 +0200, J.B.C.Engelen@...1578... wrote:
This is why I have proposed to replace assertions with exceptions in lib2geom (and already did a few). Then we don't have to remove/disable the assertions, and we end up with a system that can recover on error.
My primary concern is whether or not assertion failures get hidden from the end user. Exceptions are adequate in principle for indicating invariant failures, but what usually happens is that people start writing things like:
try { // ... } catch (AnyLibraryException e) { return some_default_value; }
For "normal" exceptions, that may not always be an unreasonable approach, but exceptions resulting from the violation of an invariant should not be lumped in with "normal" exceptions.
At minimum, a failed invariant means that there is a significant bug which may result in undefined behavior, and not infrequently in C++, failure to observe an invariant is either the result or the cause of memory corruption.
For LPEs, exceptions instead of asserts have proven to be much easier to work with. Instead of getting a bug report: "when i wiggle this line somewhere special, it crashes, don't know when". I get: "for the attached SVG, I get an LPE error". (LPE 2geom exceptions are handled by simply aborting the calculation and showing the original path, so it is very easy to see when an error happens and because Inkscape doesn't crash, you can save the data and attach it to a bugreport)
The most important thing is that assertion failures are visible to the user. Crashing is a rather blunt instrument approach to doing this, so I am not opposed to more graceful ways in principle (particularly if they facilitate the collection of debugging information).
Do we have a flag in Inkscape for release/debug that can be used to #ifdef some code?
Let's please not start #ifdeffing things. The less difference there is between the release and debug codebases, the better.
For example, the parser calling code now catches the 2geom exception, outputs path string to screen and then rethrows the exception. For release only, instead of rethrowing it might want to return an empty path instead of throwing an exception.
This would be exactly the sort of sweeping under the carpet that I am concerned about. At minimum you should distinguish between simple parse errors (e.g. due to invalid input data), which *can* sometimes be reasonably treated relatively silently in this sort of way, from assertion failures, which should *always* be propagated up to the UI somehow.
If the invariant failure happens to be associated with memory corruption, you are not doing the user any favors if you hide the fact that Inkscape is now unstable and they are putting their work at risk by not saving and exiting right then.
-mental