Re: snag of G_LIKELY()

[Owen Taylor <otaylor redhat com>]

Matthias Clasen <maclas gmx de> writes:

> On Sat, 2002-11-23 at 12:16, Daniel Elstner wrote:
> > I'd prefer:
> > 
> > #define g_return_if_fail(expr) G_STMT_START{ \
> >         gint value;                          \
> >         if (expr)                            \
> >           value = 0;                         \
> >         else                                 \
> >           value = 1;                         \
> >         if (G_UNLIKELY (value))              \
> >         {                                    \
> >             g_log (G_LOG_DOMAIN,             \
> >                  G_LOG_LEVEL_CRITICAL,       \
> >                 "file %s: line %d (%s): assertion `%s' failed", \
> >                 __FILE__,                                       \
> >                 __LINE__,                                       \
> >                 __PRETTY_FUNCTION__,                            \
> >                 #expr);                                         \
> >               return;                                           \
> >         };                     }G_STMT_END
> > 
> 
> And still hope for the compiler to generate equivalent code ? I have to
> admit that I consider the warning on assignments to be of doubious
> value. People who cannot trust themself to differentiate = and == are
> probably better of to to use
> 
> #define EQUALS(a,b) ((a) == (b))
> 
> or even
> 
> #define g_return_if_not_equal(a,b) g_return_if_fail((a) == (b))

Notes: 

 - I make the =/== mistake reasonably often. Understanding a 
   distinction doesn't meant that you don't make it. :-)

   The accepted coding-style way of preventing the mistake is

      g_return_if_fail (NULL == widget->parent)

   But we frown upon that in GTK+, partly because GCC does do
   a good job of catching the problem.

   Still, testing for equality in g_return_if_fail isn't *that*
   common (300/3916 in libgtk); so if we put code to handle
   it anywhere we  should put it in G_LIKELY()/G_UNLIKELY()
   directly.

 - The g++ problem can be fixed with __extension__; wen
   already rely on statement expressions working for G++
   in libgobject. (The info page for gcc does suggest
   not using statement expressions with g++, but if they
   break them, we'll need a new release of glib anyways)
 
 - The problem with bad code at -O0 can be fixed by
   not using the GCC definition of G_LIKELY()/G_UNLIKELY()
   except when __OPTIMIZE__ is defined; there is no point
   in the __builtin_expect() without optimization anyways.
 
 - Using a unlikely variable name for the temporary is 
   important to avoid -Wshadow warnings; 'int value' isn't 
   good.

So, combining the above, we get:

===
#if defined(__GNUC__) && (__GNUC__ > 2) && defined(__OPTIMIZE__)
#define _G_BOOLEAN_EXPR(expr)                   \
 __extension__ ({                               \
   int _g_boolean_var_;                         \
   if (expr)                                    \
      _g_boolean_var_ = 1;                      \
   else                                         \
      _g_boolean_var_ = 0;                      \
   _g_boolean_var_;                             \
})
#define G_LIKELY(expr) __builtin_expect (_G_BOOLEAN_EXPR(expr), 1)
#define G_UNLIKELY(expr) __builtin_expect (_G_BOOLEAN_EXPR(expr), 0)
#else
#define G_LIKELY(expr) expr
#define G_UNLIKELY(expr) expr
#endif
====

The remaining issues I'm aware of with this:

 - Can't nest them without causing -Wshadow warnings:

    G_LIKELY (G_UNLIKELY (a) || b)

   won't work. Probably doesn't come up that often.

 - There is some possibility there might be some code
   sequences or future versions of GCC in which the  
   temporary variable doesn't get optimized out.

 - A bit slower compilation time.

Regards,
                                        Owen