On Mon, May 12, 2014 at 10:01 PM, Dirk Hohndel <[email protected]> wrote: > On Mon, May 12, 2014 at 09:50:06PM -0300, Tomaz Canabrava wrote: >> > So I changed the silly C version of Q_ASSERT to at least tell us what the >> > offending id was and to print to stderr. But I'm not sure I agree with you >> > on removing the if (!dive) checks - if compiled without DEBUG this means >> > we will dereference NULL pointers if for some reason there's a stale id >> > somewhere. >> >> Yes, it will. and the app will crash. the Q_ASSERT ( only in debug >> mode, of course >> ) will also make the app crash right away. the idea of an assert is >> that: stop executing >> the application right away to so the developer can fix the problem. > > yes, of course. > >> The ID thing, we are not making up id's for the application, we are >> getting dives that >> gives us ID's, and using it. *if* we are forgetting to clean that id >> after a dive is deleted , >> removed or anything, it's an application bug and a crash right away is >> a good way to >> let is track it down, instead of masking it with a early return of a >> function that should >> print something, but notthing appears. > > I disagree. Crashing in a release build is NOT a good way to let the user > know that something is wrong. We should avoid crashes as much as humanly > possible. > >> > I would have to go back through the commit history to figure out if these >> > were spots were this could conceivably happen - but in general I'm not >> > sure I like the idea that we simply crash if we get an unexpected error >> > somewhere. I'd rather figure out ways to keep going (which is what the old >> > code did). >> > >> > Can you comment on that, Tomaz? >> > >> > I took the patch and just pushed it - so we may have to go in and add the >> > null-ness checks back in. >> >> See if my last comment makes sense :) >> I think it does, but I'm a weird one ;p > > Yes, you are. I much rather have us recover gracefully and tell the user > in a message window "hey, you ran into a situation that shouldn't happen, > here is some info on what happened, don't worry if this doesn't make > sense, just open a bug report at trac.hohndel.org, cut and paste this info > an we'll take a look. for now you can just keep going." and then print out > what happened where and whatever else in useful debug info we can give > ourselves.
We can achieve that by throwing an error. but since we didn't hit any Q_ASSERT till now on the code I think we are pretty safe. :) or if you prefer I can readd the check for nulls. > > /D _______________________________________________ subsurface mailing list [email protected] http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
