-
Notifications
You must be signed in to change notification settings - Fork 171
Supporting Union in C and LLVM backend #1126
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
Conversation
integration_tests/union_01.py
Outdated
class u_type(Union): | ||
_fields_ = ("integer32", ctype(i32)), ("real32", ctype(f32)), ("real64", ctype(f64)), ("integer64", ctype(i64)) |
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.
This is just syntax (so the least important part of the PR), but isn't this more readable:
class u_type(Union): | |
_fields_ = ("integer32", ctype(i32)), ("real32", ctype(f32)), ("real64", ctype(f64)), ("integer64", ctype(i64)) | |
@ccall | |
class u_type(Union): | |
integer32: i32 | |
real32: f32 | |
real64: f64 | |
integer64: i64 |
The @ccall
is telling LPyhon that this u_type
Union has to be interoperable with C. Without the decorator it does not have to be.
Then Union I think doesn't care about the order of the elements (but if it does, we'll use the order as written, just like for a struct).
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.
I think this might be using some ctypes
style. I would use our own simpler style, just like with the @ccall
decorator, where we use our own natural style using Python annotations, but then underneath we implement it using ctypes
.
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.
LPython can work with any syntax. We need it to work with CPython, so if ccall
syntax works with CPython I will also prefer it.
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.
I updated the syntax and ltypes.py
to make CPython work with this new syntax.
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.
I think the new syntax looks much better, thanks!
161502d
to
2eb1c7a
Compare
class D(Union): | ||
a: A | ||
b: B | ||
c: C |
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.
Let's also test (probably in a new test file) a Union that is not c interoperable:
class D(Union):
dx: i64
dy: f64
dz: f64
Btw, I think this union feature can also be used for common blocks and equivalence in LFortran, see lfortran/lfortran#718.
Great job, I think this looks good. We might need to update / change the syntax if needed, but to the core functionality it seems it should work. How do you create a Union that is not c interoperable? Or is this mean to be C interoperable? It seems, we might need to use # Regular pure Python Union (underneath the `@union` decorator might need to use ctypes to implement this)
@union
class D:
dx: i64
dy: f64
dz: f64
# C interoperable union, can be passed to a function implemented in C
@ccall
@union
class D:
dx: i64
dy: f64
dz: f64 |
Makes sense. Will update tomorrow. |
@certik Done. |
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.
Otherwise good.
Oh that's a great addition! |
#1113