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

[Robert Bradshaw]

On Jul 28, 2007, at 9:00 PM, Andrew Bennetts 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 agree, in fact the patch I submitted earlier today does just that.  
You can still turn it off by editing Options.py (makes it easier to  
test with vs. without code for developing) but it will be on by  
default and will generate non-GCC specific code.

- Robert