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. > > 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. -- Francois Gouget <fgouget@xxxxxxxxxxxxxxx> _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel