Ball of string -> g_return_* macros in static functions - lots of them :(

[John Rice <John Rice Sun COM>]

Hi people,

Great to see we have a performance alias.

A question has come up that I'd like people's opinions on.

In Gnome Terminal:
Bug #318891: vte module in vte.c is using g_return_val_if_fail and 
g_return_if_fail macros to validate elements in static functions.
With a data listing on an 80x55 terminal window I get a performance hit 
of 8% due to repeated calls in the static private function 
libvte:vte_terminal_find_charcell() to g_return_if_fail macro -> 
triggering calls to lookup_type_node().
So far so good, if I look at this static function, I see that the 
parameter being checked in the macro has already been checked by one 
caller and not by another, so it's a redundant check in one case and not 
in another. The safest approach is to replace it with g_assert.

But then I started to look a little more at vte.c. In this file I see 
the use of g_return_* type macros in lots of static functions [97 
instances of g_return_* macros in static functions in all]. I assume all 
of them should really be g_assert macros or indeed removed if they are 
not serving any useful internal state check.

So with this situation we will always pay the performance penalty AND we 
don't have the benefit of assert's blowing up in the internal library 
static functions to warn us during development that something is 
screwed. The g_return_* macros will allow us to fail gracefully, 
essentially masking many pathological conditions that we really ought to 
catch.

Not sure but presumably this could be one of the reasons that turning 
off all debug currently breaks the Gnome stack so badly. We should be 
able to leave in the checks on entry to the public library functions, 
but turn off internal state checks in the libraries for production.

How to fix:

Maintainers review their modules, replacing g_retun_* macros in static 
functions with g_assert calls, or optionally removing them entirely if 
they think they add no value. Then run and test the modules with 
G_DISABLE_ASSERT=1 and then 0 to make sure we are ok. Distros can 
continue to test debug builds to confirm all is well and in production 
builds disable the asserts on reviewed modules to get the performance 
gain without any risk of breaking code.

I'm happy to do it for gnome_terminal to get things rolling.

What do people think? Have a Gnome - Assert yourself day ;)

JR