Skip to content

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

Merged
merged 8 commits into from
Sep 23, 2022
Merged

Conversation

czgdp1807
Copy link
Collaborator

Comment on lines 3 to 4
class u_type(Union):
_fields_ = ("integer32", ctype(i32)), ("real32", ctype(f32)), ("real64", ctype(f64)), ("integer64", ctype(i64))
Copy link
Contributor

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:

Suggested change
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).

Copy link
Contributor

@certik certik Sep 21, 2022

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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!

@czgdp1807 czgdp1807 force-pushed the unions01 branch 2 times, most recently from 161502d to 2eb1c7a Compare September 21, 2022 10:41
@czgdp1807 czgdp1807 marked this pull request as ready for review September 21, 2022 10:52
@czgdp1807 czgdp1807 requested a review from certik September 21, 2022 10:52
class D(Union):
a: A
b: B
c: C
Copy link
Contributor

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.

@certik
Copy link
Contributor

certik commented Sep 21, 2022

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 ctypes underneath to emulate unions even in Python, even if they are not c interoperable. If so, then I would change the syntax to the following:

# 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

@czgdp1807
Copy link
Collaborator Author

Makes sense. Will update tomorrow.

@czgdp1807
Copy link
Collaborator Author

Makes sense. Will update tomorrow.

@certik Done.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

Otherwise good.

@czgdp1807 czgdp1807 enabled auto-merge September 23, 2022 04:31
@czgdp1807 czgdp1807 merged commit fae10c3 into lcompilers:main Sep 23, 2022
@czgdp1807 czgdp1807 deleted the unions01 branch September 23, 2022 04:46
@konradha
Copy link

Oh that's a great addition!

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

Successfully merging this pull request may close these issues.

3 participants