[Pyrex] Fwd: [sage-devel] Re: Cython-0.9.6

[Andrew Bennetts]

William Stein wrote:
[....]
> 
> The problem with "likely" and "unlikely" is actually taken into
> consideration (see line 2606 in Nodes.py):
> 
> if Options.gcc_branch_hints:
>     branch_prediction_macros = \
>     """
> #define likely(x)   __builtin_expect(!!(x), 1)
> #define unlikely(x) __builtin_expect(!!(x), 0)
>     """
> else:
>     branch_prediction_macros = \
>     """
> #define likely(x)   (x)
> #define unlikely(x) (x)
>     """

It is much much better to put this "if gcc" logic in the C macros themselves,
and so produce an identical C file regardless of platform.  This way
distributors of code that uses Pyrex (or Cython) can generate the C code when
they make a release, and package it the source tarball.  You're effectively
either requiring that people using Cython can't pre-generate the C code, making an
extra dependency for their users, or requiring a different source tarball for
different platforms!

Also, it will considerably ease future debugging on mailing lists and bug
reports if you don't have to keep asking about "And did you compiler-XYZ
option?"  It will make it harder to compare output of one version of Cython from
the next when trying to track down bugs, because the reporter might have have
compiled for a different compiler.  And the user experience is better, because
it will Just Work with any standards-compliant compiler.

There's just no reason not to do this in the macros that I can think of.  C
already gives you ample tools to do this portably, why reinvent them in a less
appropriate layer?  You are producing a .c file, it should be C99 compatible,
rather than specific to a particular vendor.

So use "#if __GNUC__", or maybe even check for a specific version.  See
<http://gcc.gnu.org/onlinedocs/gcc-4.2.1/cpp/Common-Predefined-Macros.html>.  A
quick check of the GCC manuals show that __builtin_expect has existed since at
least gcc 3.4.0.

-Andrew.