cython and C++

[vbraun]

I still can't build C++ cython modules, even though it should be possible with the cython-0.13.

python `which cython` --embed-positions --directive cdivision=True,autotestdict=False -I/home/vbraun/opt/sage-4.5.3/devel/sage-main -o sage/geometry/ppl.cpp sage/geometry/ppl.pyx

Error converting Pyrex file to C:
------------------------------------------------------------
...
    """   

    cdef Variable *thisptr
      
    def __cinit__(self, dimension_type i):
        thisptr = new Variable(i)
                     ^
------------------------------------------------------------

/home/vbraun/opt/sage-4.5.3/devel/sage-main/sage/geometry/ppl.pyx:19:22: Operation only allowed in c++

The problem seems to be that cython expects --cplus to enable the new language features. Cython does not derive this from the output file name extension ('cpp').

I've written the obvious patch to setup.py, see attachment.

Apply trac_10094_add_cplus_command_line_option.patch​ and 10094_fix_doctests.2.patch​.

[vbraun]

Initial patch

[vbraun]

python `which cython` --cplus --embed-positions --directive cdivision=True,autotestdict=False -I/home/vbraun/opt/sage-4.5.3/devel/sage-main -o sage/rings/finite_rings/element_givaro.cpp sage/rings/finite_rings/element_givaro.pyx

Error converting Pyrex file to C:
------------------------------------------------------------
...
                    modulus.append(0)
            
        if is_Polynomial(modulus):
            modulus = modulus.list()

        if PY_TYPE_CHECK(modulus, list) or PY_TYPE_CHECK(modulus, tuple):
                       ^
------------------------------------------------------------

/home/vbraun/opt/sage-4.5.3/devel/sage-main/sage/rings/finite_rings/element_givaro.pyx:264:24: ambiguous overloaded method

Givaro, one of the C++ extension modules, is very unhappy about cython --cplus.

[vbraun]

The problem is this snippet from element_givaro.pyx, where PY_TYPE_CHECK is declared a second time. It is then ambiguous with its first declaration.

cdef extern from "stdsage.h":
    [...]
    int PY_TYPE_CHECK(object o, object t)

Removing this one line works, patch attached. I did a sage -ba and had no more troubles.

[vbraun]

Initial patch

[vbraun]

Final patch for minor doctest breakage. The two doctests fixes to sageinspect.py also need to be applied to local/lib/python2.6/site-packages/sagenb-0.8.2-py2.6.egg/sagenb/misc/sageinspect.py. I'll put a corresponding comment on #10036.

[vbraun]

Initial patch

[jason]

The colormaps doctest is fixed in the patch on the matplotlib upgrade ticket, and shouldn't be included in this patch (it is already merged).

[jason]

Should we apply all three patches?

[jason]

I think the sagenb issue is already fixed as well: ​http://trac.sagemath.org/sage_trac/attachment/ticket/9828/trac_9828-sagenb_cython_0_13.patch

[jason]

So I guess my point is that trac_10094_fix_doctests.patch is not needed, as those fixes are already merged on #10036 (the notebook issue) and the matplotlib upgrade to 1.0.

[vbraun]

Oh, I didn't know that the doctest fixes are already included elsewhere. So, just to be sure, apply

1. trac_10094_add_cplus_command_line_option.patch
2. trac_10094_fix_givaro.patch

[jason]

robertwb: can you comment on this ticket? Does --cplus switch on the capability, or does it force c++ generation (i.e., should --cplus be enabled for all files??)

[jason]

Can you not just declare the language to be c++ in the module_list.py file, instead of modifying the command-line switch for everything?

[jason]

In fact, it seems that lots of modules in module_list.py already switch on the C++ features by doing language="c++"

[vbraun]

Initially I of course used language="c++", this does not work. If you look at the original patch, all that does is switch the output file name extension to cpp. And cython then still relies on the --cplus switch. I don't know if thats a cython bug or intentional design. Robert, you should be able to tell us?

Also, note that the patch only adds --cplus if language="c++" in module_list.py

[jason]

Replying to vbraun:

Also, note that the patch only adds --cplus if language="c++" in module_list.py

Okay, thanks for the clarification.

[robertwb]

The --cplus is needed to use C++ features in the Cython file, and language="c++" is needed to invoke the C++ compiler. We should not enable --cplus for all files, as it may (now or in the future) emit C++ code. The reason we need to specify the flag manually is that we're manually invoking Cython (for good reason) rather than trying to let distutils do it for us. I thought, however, that this setting would be inferred from the output file extension, apparently not (yet/anymore?).

In any case, positive review to trac_10094_add_cplus_command_line_option.patch

[jdemeyer]

Patches trac_10094_fix_givaro.patch and trac_10094_fix_doctests.patch give patch errors. I will check to see whether they are still needed.

[vbraun]

The givaro patch is obsoleted by #10089 which removed even more unnecessary includes.

[jdemeyer]

Updated doctest patch

[jdemeyer]

Can somebody review 10094_fix_doctests.2.patch, then we have a full positive_review.

[robertwb]

10094_fix_doctests.2.patch looks good to me.