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

[William Stein]

On 7/28/07, Andrew Bennetts <andrew-pyrex at puzzling.org> wrote:
> 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.
>

I'm convinced.  Can you send me a patch?

 -- William