-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix "except Exception" in limited api #5699
Conversation
Partial fix for cython#5697 Change cname for builtin types to be a pointer rather than a value. This means that we can avoid defererencing BaseException to transform it into a value (which doesn't work on the limited api, because its an opaque struct)
Unfortunately this doesn't look like it;s going to work in this form. I think the principle is good. Just unclear how much code will need changing to make it work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but refactorings like this always feel risky for a point release. Should we do this in Cython 3.1?
Cython/Compiler/TypeSlots.py
Outdated
def slot_code(self, scope): | ||
dict_entry = scope.lookup_here("__dict__") if not scope.is_closure_class_scope else None | ||
if dict_entry and dict_entry.is_variable: | ||
if getattr(dict_entry.type, 'cname', None) != 'PyDict_Type': | ||
if getattr(dict_entry.type, 'typeptr_cname', None) not in ['&PyDict_Type', '(&PyDict_Type)']: | ||
error(dict_entry.pos, "__dict__ slot must be of type 'dict'") | ||
return "0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the fault of this PR, but it seems very late do detect this case here, during code generation.
But shouldn't we just check is Builtin.dict_type
here? Why look at the type pointer?
Probably sensible. I don't think there's an alternative way of way of the issue but there's plenty of other things broken in the limited API anyway.
I'd assumed it was too do with letting you also use a |
In the absence of any objections I'm going to treat this as "approved for 3.1" and merge it in fairly shortly in the hope that any regressions will be caught early. |
Partial fix for #5697
Change cname for builtin types to be a pointer rather than a value. This means that we can avoid defererencing BaseException to transform it into a value (which doesn't work on the limited api, because its an opaque struct)