Patches

[Tomaz Canabrava]

On Mon, May 12, 2014 at 9:31 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
> On Mon, May 12, 2014 at 04:10:05PM -0300, Tomaz Canabrava wrote:
>> From 8a517fcd926a7ecb8d2ef8afb91da766f591b876 Mon Sep 17 00:00:00 2001
>> From: Tomaz Canabrava <tomaz.canabrava at intel.com>
>> Date: Mon, 12 May 2014 14:22:46 -0300
>> Subject: [PATCH 4/5] Renomed getDiveById to get_dive_by_id to keep current c
>>  code organized.
>>
>> This commit renames getDiveById to get_dive_by_id, and it also removes
>> the Q_ASSERTS and if(!dive) return that the callers of this function
>> were calling. if it has a Q_ASSERT this means that the dive must exist,
>> so checking for nullness was bogus too. I'v changed the assert ( done
>> in a silly C-Way, someone that understands a bit more of C should try
>> to fix it for me ) to the function call to simplify code.
>
> 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.

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 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

Tomaz
> /D