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 haxe.runtime.Copy #11863

Merged
merged 11 commits into from
Dec 16, 2024
Merged

Add haxe.runtime.Copy #11863

merged 11 commits into from
Dec 16, 2024

Conversation

Simn
Copy link
Member

@Simn Simn commented Dec 9, 2024

From #11857.

Needs more tests and such.

Closes #11862.

@skial skial mentioned this pull request Dec 9, 2024
1 task
@ALANVF
Copy link
Contributor

ALANVF commented Dec 10, 2024

I'm excited for this new addition, although I noticed that copyValue is Dynamic. Is there a reason why it could not be made generic instead as copyValue<T>(value: T): T? It works the same way while still preserving type safety

@Simn
Copy link
Member Author

Simn commented Dec 10, 2024

The reason is mostly that I copied this from Nicolas. And yes we should definitely change it to copyValue<T>(value: T): T here.

@Simn
Copy link
Member Author

Simn commented Dec 11, 2024

There are two situations related to __id__ handling on JS/neko:

  1. The Serializer and the original implementation do some nasty looking Reflect.deleteField(k, "__id__") stuff when handling ObjectMap keys. After looking at this a bit I don't see why this is necessary because the implementation of Reflect.fields on both JS and neko already deals with __id__ fields, so they will not be serialized/copied.

  2. With the cache being an ObjectMap now, we end up adding __id__ fields to keys:

function main() {
	var obj1 = {};
	var obj1Copy = haxe.Copy.copy(obj1);
	trace(obj1); // { __id__: 0 }
	trace(obj1Copy); // {}
}

We could look into removing these values after copying, but I don't know if that would actually improve anything. Ultimately this happens for any implementation that uses ObjectMap, so maybe it's acceptable to leave this as-is.

Edit: I've changed it to not use ObjectMap on neko and JS, so there is no __id__ business now.

@Simn
Copy link
Member Author

Simn commented Dec 11, 2024

I took another look at the enum parameter handling of the original implementation and decided to forego it. It was implemented for neko, php and js and as it turns out, it actually wasn't working on JS unless -D js-enums-as-arrays was set. And I don't think we need an implementation that works only on neko and php.

This means that the copy implementation will also run into what I outlined in #11865. This isn't ideal, but it's also not fatal because enum values cannot be recursive by themselves, and so at worst they end up appearing as multiple instances.

Edit: This is now addressed by deferring all recursions. This ensures the enum value instance is cached before anyone else can come across it again.

@Simn
Copy link
Member Author

Simn commented Dec 11, 2024

This should be fine now. I'm sure it can be optimized more but it's a good start.

The remaining question is where to actually put this. Having haxe.Copy is a bit random, but I don't want to add it to some other class and I don't know if it makes sense to open a new haxe.runtime package or whatever.

@Simn Simn changed the title Start working on haxe.Copy Add haxe.runtime.Copy Dec 16, 2024
@Simn
Copy link
Member Author

Simn commented Dec 16, 2024

I've created the haxe.runtime package after all, this might be a good place to put various lowish-level target runtime support code.

@Simn Simn merged commit c85b00d into development Dec 16, 2024
98 of 100 checks passed
@Simn Simn deleted the haxe.Copy branch December 16, 2024 17:12
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.

Add clone functionality to std
2 participants