Skip to content

Add unused type parameter to retain and release to prepare for closures #1287

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

Conversation

DuncanUszkay1
Copy link
Contributor

Extracting some no-op changes from my Closures PR to make it easier to parse.

What's been extracted is just the change to have type information passed into retain/release in any scenario where the expression could refer to a function pointer.

Context behind this change:

While none of the below is actually present in this change, which does nothing, it explains what this change will be used for in the closures implementation.

With our current scheme of altering function references so that they can hold closures or normal function references, we need to know the type of a block in order to know how to retain or release it.

  • When the type is a function type, we check the MSB, and if it's set we retain/release a shifted value. If it isn't, we retain/release 0, which is a no-op.
  • When the type isn't a function, retain/release as normal

@DuncanUszkay1

This comment has been minimized.

@DuncanUszkay1 DuncanUszkay1 mentioned this pull request May 23, 2020
@DuncanUszkay1
Copy link
Contributor Author

@dcodeIO thoughts on this? If this is okay to go in then it'd be great to have it in sooner rather than later since it'll help people review the closures PR

@dcodeIO
Copy link
Member

dcodeIO commented May 25, 2020

Yes, this change looks good, going to take a look at the PR now.

Copy link
Member

@dcodeIO dcodeIO left a comment

Choose a reason for hiding this comment

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

Generally looks good to make this change, just a few comments. Not all of the comments are strictly related to your change, but some also propose to establish a proper convention for the helpers, in that whenever a raw expression without a type is taken, the corresponding type must be provided as well. Does that sound good to you?

@DuncanUszkay1
Copy link
Contributor Author

in that whenever a raw expression without a type is taken, the corresponding type must be provided as well. Does that sound good to you?

Makes sense to me. If it's a more general thing though let's put it into a separate issue.

@DuncanUszkay1
Copy link
Contributor Author

I likely haven't added the type everywhere, but everywhere which calls retain/release which feels appropriate for this PR, and then maybe spin out an issue if we feel like we need the rest?

@DuncanUszkay1
Copy link
Contributor Author

Looks like I'm failing the new lint now?

@MaxGraey
Copy link
Member

MaxGraey commented May 26, 2020

No, it's still old tslint currently. Just replace top-level functionlet declaration (which not inside blocks) to var

@DuncanUszkay1 DuncanUszkay1 force-pushed the add-type-to-retain-release branch from 52ed016 to d61b7ea Compare May 26, 2020 15:21
@DuncanUszkay1
Copy link
Contributor Author

Should be good to go now

@MaxGraey
Copy link
Member

@dcodeIO could you look?

@dcodeIO dcodeIO merged commit 2937480 into AssemblyScript:master May 27, 2020
@dcodeIO
Copy link
Member

dcodeIO commented May 27, 2020

Thanks, rechecked and looks good!

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