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

Add ReferenceStorage for manual allocation of references #14106

Merged

Conversation

HertzDevil
Copy link
Contributor

This adds the Instance type mentioned in #13481. The naming is not final.

@HertzDevil HertzDevil changed the title Add ReferenceStorage(T) for manual allocation of T Add ReferenceStorage for manual allocation of references Dec 17, 2023
src/compiler/crystal/types.cr Outdated Show resolved Hide resolved
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

LGTM.

As mentioned in the OP, naming is still kind of preliminary. But in absence of any really clever idea, we can just take ReferenceStorage.
In case there is something inherently wrong about it, we can still change it as it's marked as experimental.

@straight-shoota straight-shoota added this to the 1.11.0 milestone Dec 21, 2023
@HertzDevil
Copy link
Contributor Author

What about the minimal implementations for Object's abstract defs? Do you think they make sense?

@straight-shoota
Copy link
Member

straight-shoota commented Dec 21, 2023

Not sure... Maybe we could leave them out for now and add them later if we notice they're missing? It's easy to patch them in ad-hoc, so this isn't holding up anything.

I suppose instances of ReferenceStorage wouldn't really be used on their own. The most typical use case we have been thinking of (so far):

just_some_variable_to_put_this_on_the_stack = uninitialized ReferenceStorage(Foo)
foo = Foo.pre_initialize(pointerof(just_some_variable_to_put_this_on_the_stack))

And just_some_variable_to_put_this_on_the_stack is never used again (unless you want to allocate a something else in that storage, but that would be the same usage as the second line here).

Probably, this pattern will be abstracted away in a macro for convenience, further reducing direct usage of ReferenceStorage.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Dec 21, 2023

The problem is #==, #to_s and #hash cannot be left out at all because the type inherits directly from Value rather than Struct. (I guess you could leave out #== because Value has a catch-all def that returns false)

@straight-shoota
Copy link
Member

Sounds good. Let's keep it then 👍

@straight-shoota straight-shoota merged commit 15010d3 into crystal-lang:master Dec 23, 2023
56 checks passed
@HertzDevil HertzDevil deleted the feature/reference-storage branch December 27, 2023 12:53
straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Jan 10, 2024
@straight-shoota
Copy link
Member

straight-shoota commented Jan 10, 2024

This was eventually reverted in #14207 for 1.11.1 due to #14194. An alternative impementation is available at #14204.

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

Successfully merging this pull request may close these issues.

3 participants