Conversation
| def declaration_code(self, entity_code, | ||
| for_display = 0, dll_linkage = None, pyrex = 0): | ||
| # XXX: we put these guards in for now... | ||
| assert not pyrex |
There was a problem hiding this comment.
I guess the call in line 3229 of the same file is the culprit here. There is a special-case for for_display just below. The question is how pyrex=True should behave there (read: the declaration code in a Cython-specific context).
What happens if you create multiple wrappers for functions with slightly different memoryview arguments? Do we build suitable C names for all of them?
There was a problem hiding this comment.
The call in line 3229 is the culprit.
Multiple wrappers for functions create multiple wrappers with the same name that all conflict with each other. Adding for_display = for_display or pyrex creates something that compiles (but probably doesn't really do what pyrex implies it should). I'll come back to this in the individual PR.
There was a problem hiding this comment.
pyrex=True is usually very similar to for_display=True, since both target the Cython code level. See the comment near the top of the file in class PyrexType. Can't say right now what a good escaped version of a memoryview declarations for use in a C name should look like. Probably needs some trial and error.
| if atype.is_fused and env.fused_to_specific: | ||
| atype = atype.specialize(env.fused_to_specific) | ||
| if as_target and env.is_c_class_scope and not atype.is_pyobject: | ||
| # TODO: this will need revising slightly for cdef dataclasses when implemented |
There was a problem hiding this comment.
And probably also if we decide to allow the annotation syntax for defining cdef attributes. That's not entirely out of focus.
| # specialize can also go through layers of pointers/const which makes anything other | ||
| # than "try...except" difficult |
There was a problem hiding this comment.
That suggests to me that this might not be the right place to do this. Can't we detect this earlier? E.g. when analysing the function signature?
There was a problem hiding this comment.
It probably could be done earlier but I think the problem would be the same - to work out if it specializes then you have to duplicate most of the specialization logic in PyrexTypes.
I wonder if it'd be better if specialize returned error_type rather than raising an exception to allow for "safe" checks that something specializes?
There was a problem hiding this comment.
Isn't it an issue whenever a fused type occurs in the return type that isn't used in any of the arguments? Or are there legitimate use cases for that as well?
|
Given that I made three different comments about completely distinct behaviours and changes in this PR, I think separate PRs seem more appropriate. The fact that all three changes improve the warnings feels less relevant than the difference in behaviour that these changes create. |
|
I'll close this and submit them separately then. |
This fixes a few recently reported compiler crashes. It should possibly be 3 separate PRs and I can split it up if needed.
Fixes #3843 - allow creation of wrapper functions with memoryview arguments
Fixes #3820 - report error when return type cannot be deduced from fused arguments
Fixes #3830 - don't allow annotation types to set the type of cdef class class-level attributes