On Mon, 2015-11-23 at 16:20 +0100, Francois Gouget wrote: > On Fri, 20 Nov 2015, Christophe Fergeau wrote: > [...] > > > 5. And you propose adding spice_return_if_fail_warning() to fix this mess. > > > > > > I really don't see how adding more functions is going to make this less > > > confusing and error prone! Particularly if there is not a concerted > > > and swift effort to clean up the old code. > > > > If we were to rename spice_return_if_fail() to spice_assert_if_fail() or > > such, a global replace and a git rebase -x "sed -i > > s/spice_return_if_fail()/spice_assert_if_fail()/gc" would indeed be in > > order. > > If not doing a straight replace with g_return_if_fail() I think > this would be a good first step and clarify the code. Yeah, that seems to be a reasonable initial step to me as well. > > > > > Note that spice_error() needs to be fixed too. That name implies the > > > function logs an error just like spice_warning() logs a warning, not > > > that it crashes the application. spice_error() should be renamed to > > > spice_fatal(). For consistency it might make sense to rename > > > SPICE_LOG_LEVEL_ERROR to SPICE_LOG_LEVEL_FATAL. > > > > spice_error() is consistent with g_error() here: > > https://developer.gnome.org/glib/2.46/glib-Message-Logging.html#g-error > > "Error messages are always fatal, resulting in a call to abort() to > > terminate the application. This function will result in a core dump; > > don't use it for errors you expect. Using this function indicates a bug > > in your program, i.e. an assertion failure." > > Following the glib convention does make sense. I do disagree with glib's > naming choice though but there's nothing that can be done about that at > this point. > > > [...] > > NB: most of the time g_free() and free() are equivalent (they always are > > with newer glib). Very not clean to mix and match g_malloc/free, but > > nothing terribly wrong should happen either. > > That may be true right now but does glib explicitly condone it? As long > as you use g_alloc()+g_free() they can change their implementation as > they want and your code will still work. > > That's what bothers me with spice_malloc(): such pointers can only be be > freed with plain free(). It hardcodes the assumption that spice_malloc() > is a wrapper around plain malloc() everywhere in the code, making it > impossible to switch to some other allocator. At least not until every > user of spice-common has been fixed. > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel