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.

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

cgo: refactor #2774

Merged
merged 1 commit into from
May 6, 2022
Merged

cgo: refactor #2774

merged 1 commit into from
May 6, 2022

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Apr 13, 2022

This is a large refactor of the cgo package. It should fix a number of smaller problems and be a bit more strict (like upstream CGo): it for example requires every Go file in a package to include the header files it needs instead of piggybacking on imports in earlier files.

The main benefit is that it should be a bit more maintainable and easier to add new features in the future (like static functions).

This breaks the tinygo.org/x/bluetooth package (tinygo-org/bluetooth#97), which should be updated before this change lands.

This is a large refactor of the cgo package. It should fix a number of
smaller problems and be a bit more strict (like upstream CGo): it for
example requires every Go file in a package to include the header files
it needs instead of piggybacking on imports in earlier files.

The main benefit is that it should be a bit more maintainable and easier
to add new features in the future (like static functions).

This breaks the tinygo.org/x/bluetooth package, which should be updated
before this change lands.
aykevl added a commit to tinygo-org/bluetooth that referenced this pull request Apr 13, 2022
@aykevl
Copy link
Member Author

aykevl commented Apr 13, 2022

This PR changes the internal structure of the cgo package:

  • Instead of scanning the Go files for C references and then scanning the C files for declarations, the order is reversed: all C declarations are scanned and then the Go AST is scanned for C.foo symbols to replace. This should fix some issues with the const parser in a future change.
  • CGo is processed independently per Go file and duplicate definitions are deduplicated as needed. This matches upstream CGo.

deadprogram pushed a commit to tinygo-org/bluetooth that referenced this pull request Apr 13, 2022
@deadprogram
Copy link
Member

CGo is processed independently per Go file and duplicate definitions are deduplicated as needed. This matches upstream CGo.

💯

@deadprogram
Copy link
Member

This is a large refactor of the cgo package.

You do not exaggerate. I am still digesting it, but so far I like what I see.

@deadprogram
Copy link
Member

I suspect a change may be needed for https://github.com/tinygo-org/tinyfs

@aykevl
Copy link
Member Author

aykevl commented Apr 13, 2022

I suspect a change may be needed for https://github.com/tinygo-org/tinyfs

Didn't test it, but yes there's a good chance that package needs an update.

@deadprogram
Copy link
Member

Seems like tinyfs is still working as expected even with this refactor. I think we should merge unless for some reason we want to hold until after next release.

@deadprogram
Copy link
Member

Let's merge this now. Thanks @aykevl for the good work!

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.

2 participants