Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't cimport type #18321

Closed
jdemeyer opened this issue Apr 28, 2015 · 10 comments
Closed

Don't cimport type #18321

jdemeyer opened this issue Apr 28, 2015 · 10 comments

Comments

@jdemeyer
Copy link

This line in src/sage/ext/stdsage.pxd

from cpython.type cimport type

causes trouble as it causes Cython to no longer optimize the type type.

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 459908c

Reviewer: Peter Bruin

Issue created by migration from https://trac.sagemath.org/ticket/18321

@jdemeyer jdemeyer added this to the sage-6.7 milestone Apr 28, 2015
@jdemeyer
Copy link
Author

Branch: u/jdemeyer/don_t_cimport_type

@jdemeyer
Copy link
Author

New commits:

5d4b11eDon't cimport "type"

@jdemeyer
Copy link
Author

Commit: 5d4b11e

@pjbruin
Copy link
Contributor

pjbruin commented May 9, 2015

comment:3

What is the reason for still doing cimport type as pytype in stdsage.pxd? Removing this line (and using type instead of pytype in PY_NEW) does not seem to cause any problems at first sight. Does it have any speed and/or type safety effects?

@pjbruin
Copy link
Contributor

pjbruin commented May 9, 2015

Reviewer: Peter Bruin

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2015

Changed commit from 5d4b11e to 459908c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

459908cUse plain "type", not "cpython.type.type" for PY_NEW()

@jdemeyer
Copy link
Author

jdemeyer commented May 9, 2015

comment:5

Replying to @pjbruin:

What is the reason for still doing cimport type as pytype in stdsage.pxd?

No idea. It must have been needed at some point, but maybe some Cython upgrade changed this?

@pjbruin
Copy link
Contributor

pjbruin commented May 9, 2015

comment:6

Replying to @jdemeyer:

Replying to @pjbruin:

What is the reason for still doing cimport type as pytype in stdsage.pxd?

No idea. It must have been needed at some point, but maybe some Cython upgrade changed this?

Who knows. The other occurrence (in classcall_metaclass.pyx) does seem to be necessary; without it, there is a doctest that crashes Sage due to insufficient type checking.

@vbraun
Copy link
Member

vbraun commented May 9, 2015

Changed branch from u/jdemeyer/don_t_cimport_type to 459908c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants