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.

Sign up for GitHub

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

Jump to bottom

Avoid using strings for exception_value of func_type #5710

Merged
merged 14 commits into from
Mar 7, 2024
Merged

Avoid using strings for exception_value of func_type #5710

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4c0d095
Avoid using strings for exception_value of func_type
da-woods Sep 17, 2023
3f29ce4
Fix Cython self-build hopefully
da-woods Sep 17, 2023
f3f3dd1
Shift the exception_value logic into ExprNode
da-woods Sep 17, 2023
dab7347
Fix codegen when exception_value == 0
da-woods Sep 17, 2023
b9fe05c
Remove debugging code
da-woods Sep 17, 2023
63dc932
Fixup remaining tests (hopefully)
da-woods Sep 17, 2023
9417b32
Fix BoolNode compile-time value
da-woods Sep 17, 2023
9666e11
Avoid boolnode
da-woods Sep 17, 2023
30325f0
Merge branch 'master' into avoid-strs-for-functype-exception-value
da-woods Nov 13, 2023
2562ad2
Comments from review
da-woods Dec 16, 2023
0ffd487
Assume constant result is already calculated
da-woods Dec 16, 2023
1abb981
Apply suggestions from code review
da-woods Dec 16, 2023
9dd95fa
More review stuff
da-woods Dec 16, 2023
e49988c
Merge branch 'master' into avoid-strs-for-functype-exception-value
da-woods Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions Cython/Compiler/ExprNodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1311,6 +1311,26 @@ def calculate_result_code(self):
def generate_result_code(self, code):
pass

@staticmethod
def make_specific_const_node(pos, value, type):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably build this automatically from the subclasses of ConstNode and their types. That reduces the chance of forgetting to update this method. You forgot str_type, at least.
https://stackoverflow.com/questions/3862310/how-to-find-all-the-subclasses-of-a-class-given-its-name

I think a less redundant name would be ConstNode.for_type().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringNode is a subclass of PyConstNode rather than ConstNode so wouldn't be caught by the subclasses check. I also think it's difficult to do while still supporting Python 2.

I've changed the name though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's difficult to do while still supporting Python 2.

We are looking at the Cython literal type here, right? Not the Python object type? Then it's easy to do because we see these types in the source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I more meant the __subclasses__ mechanism that is used by the stack overflow page that you linked doesn't exist in Python 2, so it's difficult to get a list of all the subclasses of ConstNode

cls = ConstNode
if type is PyrexTypes.c_bint_type:
cls = BoolNode
elif type is PyrexTypes.c_null_ptr_type or (
(value == "NULL" or value == 0) and type.is_ptr):
return NullNode(pos) # value and type are preset here
elif type == PyrexTypes.c_char_type:
cls = CharNode
elif type.is_int:
cls = IntNode
elif type.is_float:
cls = FloatNode
elif type is bytes_type:
cls = BytesNode
elif type is unicode_type:
cls = UnicodeNode
return cls(pos, value=value, type=type)


class BoolNode(ConstNode):
type = PyrexTypes.c_bint_type
Expand Down
12 changes: 7 additions & 5 deletions Cython/Compiler/Nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,8 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
# for now.
if not env.is_c_class_scope and not isinstance(self.base, CPtrDeclaratorNode):
from .ExprNodes import ConstNode
self.exception_value = ConstNode(
self.pos, value=return_type.exception_value, type=return_type)
self.exception_value = ConstNode.make_specific_const_node(
self.pos, value=str(return_type.exception_value), type=return_type)
if self.exception_value:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You changed this to is (not) None in other places, so here as well? I still see a couple of other places where we use a plain if … self.exception_value …:, e.g. in the error check just a bit up.

Suggested change
if self.exception_value:
if self.exception_value is not None:

if self.exception_check == '+':
self.exception_value = self.exception_value.analyse_const_expression(env)
Expand All @@ -759,8 +759,10 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
else:
self.exception_value = self.exception_value.analyse_types(env).coerce_to(
return_type, env).analyse_const_expression(env)
exc_val = self.exception_value.get_constant_c_result_code()
if exc_val is None:
self.exception_value.calculate_constant_result()
exc_val = self.exception_value.constant_result
from .ExprNodes import constant_value_not_set
if exc_val is None or exc_val is constant_value_not_set:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this something that ConstNode could do? That would avoid the local import. Something like ExprNode.as_exception_value(), where the default implementation would call error() and subclasses could override it.

We should also make sure that this works for enum values etc., anything that is usable as exception value.

error(self.exception_value.pos, "Exception value must be constant")
if not return_type.assignable_from(self.exception_value.type):
error(self.exception_value.pos,
Expand Down Expand Up @@ -3125,7 +3127,7 @@ def as_cfunction(self, cfunc=None, scope=None, overridable=True, returns=None, e

if exception_value is None and cfunc_type.exception_value is not None:
from .ExprNodes import ConstNode
exception_value = ConstNode(
exception_value = ConstNode.make_specific_const_node(
self.pos, value=cfunc_type.exception_value, type=cfunc_type.return_type)
declarator = CFuncDeclaratorNode(self.pos,
base=CNameDeclaratorNode(self.pos, name=self.name, cname=None),
Expand Down
22 changes: 22 additions & 0 deletions tests/run/exceptionpropagation.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,25 @@ def test_ptr_func2(int failure_mode):
print("NULL")
except RuntimeError:
print("exception")

cdef double return_double(double arg, bint fail):
if fail:
raise RuntimeError
return arg

ctypedef double (*func_ptr1)(double, bint) except? -1

def test_return_double(fail):
"""
>>> test_return_double(False)
2.0
>>> test_return_double(True)
exception
"""
# Test that we can assign to the function pointer we expect
# https://github.com/cython/cython/issues/5709 - representation of the "-1" was fragile
cdef func_ptr1 p1 = return_double
try:
return p1(2.0, fail)
except RuntimeError:
print("exception")