Patches

[Tomaz Canabrava]

On Mon, May 12, 2014 at 10:01 PM, Dirk Hohndel <dirk at hohndel.org> 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