Re: [Gtkpod-devel] [PATCH] Store references to data getters instead of using a conditional

[Jorg S. <Jor...@gm...>]

Hi Javier,

>     The whole point of g_return_val_if_fail() is to catch programming
>     errors. Compare and contrast with: if (!cts) return 0;
>
>
> Thanks for your feedback. I know what this function does, although I'm 
> not certain why it isn't an assertion. However my patch, which 
> apparently I didn't attach properly, did not remove the calls to 
> g_return_val_if_fail. It made a different refactoring. I'll send the 
> patch later today.
Christophe uses g_asserts(), I use g_return_if_fail(). The reason being 
that as we can't really avoid bugs (or unexpected iTunesDB changes) 
g_assert() will cause the program to terminate pretty much without 
feedback, while g_return_if_fail() will spit out some warnings and the 
program will usually continue to run.

>     Those can be disabled in glib.
>
>
> Thanks for your comments. It would be good to see these disabled 
> automatically in optimized/distro builds. I don't think it's the case, 
> but I don't have the Debian rules file in front of me, to pick the 
> distro I use.
I think they aren't disabled in most distros. You're right.

> These things actually are at the top of my profiling data for 
> /loading/ the iPod. These get*int functions get called a couple 
> million times in very quick progression, so there is some benefit. As 
> a user I hate to wait. As a developer, perhaps not so much, as I've 
> already spent a few hours on this, but as I user I only had to wait 
> less than a minute total, each time I used gtkpod :-)
I appreciate the effort you have put in these patches!

>     Admittedly, I didn't look at the code, and
>     g_return_* is usually only used for public interfaces, so it could
>     very
>     well be removed if the developers think it's unlikely to happen.
>
>
> Yes, in this case it's being used in the lowest level of code. The 
> program can't possibly get that far if cts is NULL anyway.
If we have g_return_if_fail() statements where it's completely 
impossible to fail, we can surely remove them now, especially if there 
is a noticable effect on loading time.

Cheers,


JCS.