[Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort

[Frediano Ziglio]

> On Fri, Mar 29, 2019 at 06:57:59AM -0400, Frediano Ziglio wrote:
> > > On Fri, Mar 29, 2019 at 11:30:46AM +0100, Christophe Fergeau wrote:
> > > > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > > > will never return.
> > > > 
> > > > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > > > ---
> > > >  common/log.h | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/common/log.h b/common/log.h
> > > > index 7c67e7a..1482358 100644
> > > > --- a/common/log.h
> > > > +++ b/common/log.h
> > > > @@ -20,6 +20,7 @@
> > > >  
> > > >  #include <stdarg.h>
> > > >  #include <stdio.h>
> > > > +#include <stdlib.h>
> > > >  #include <glib.h>
> > > >  #include <spice/macros.h>
> > > >  
> > > > @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> > > >  #define spice_return_if_fail(x) G_STMT_START {
> > > >  \
> > > >      if G_LIKELY(x) { } else {
> > > >      \
> > > >          spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC,
> > > >          "condition `%s' failed", #x); \
> > > > +        abort();
> > > > \
> > > >          return;
> > > >          \
> > > 
> > > The 'return' statment is now unreachable code & can be removed -
> > > surprised
> > > the compiler didn't complain that its unreachable.
> > > 
> > 
> > As OT I would also add that a "spice_return_if_fail" which don't return
> > is confusing, basically it's a hidden assert.
> 
> Yeah it is a rather misleading name.  I see this code is importing glib.h
> so I wonder why spice_return_if_fail needs to exist at all. Why not just
> drop it and use g_return_if_fail from glib instead of reinventing the
> wheel.  Or g_warn_if_fail if you want a log message, or g_assert if
> you want a fatal error.
> 
> https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-return-if-fail
> 
> Regards,
> Daniel

Yes, this was discussed. The behaviour is different so it makes sense to keep
them. For instance for use critical is fatal while in Glib not.
Another difference is the informations they report.

Frediano