[WiP] Adding keyValueIterator to Array

[elnabo]

See #7395

Array

• unit test
• cpp
• cs
• eval
• flash
• hl
• java
• js
• lua
• neko
• php
• python

Edit: Limit to Array

[elnabo]

@nadako didn't see your comment on #7395 before doing this commit. Hope didn't redo what you already did.

Some changes might be needed to the Array.keyValueIterator function according to what have been said there.

[Simn]

Oooh, I like where this is going!

I was thinking that we should open haxe.iterator to store some common code. That way we don't have to duplicate these iteration classes so much.

[elnabo]

That would be nice, especially since many target have different naming/style convention.

Btw, I assume that Array handling in cpp is done in hxcpp. But what about the hl and flash target ?

[Simn]

Check the Boot.hx class for flash.

And yes, both hxcpp and hl are going to need updates to their native code.

[ncannasse]

Just wanted to make sure : can we accurately make sure that the code for( idx => key in array ) will not result in any temp object being allocated in the form { key : index, value : v } ?

[RealyUniqueName]

@elnabo I can't make a PR to your fork, so here is the commit wich, I guess, illustrates what @Simn means with haxe.iterators: RealyUniqueName@3303ce3
This way no temp objects/allocations will be generated by the compiler.

[elnabo]

@RealyUniqueName Thanks for the commit. Dunno why it's not possible to PR the fork.

However for the hl target, I don't think it is possible to access the Array and thus to return something equivalent to an haxe.iterators.ArrayKeyValueIterator.

[elnabo]

I'm also unsure how the eval would use this class.

[RealyUniqueName]

We can have this in std/Array.hx:

@:pure @:runtime inline function keyValueIterator() : ArrayKeyValueIterator<T> {
	return new ArrayKeyValueIterator(this);
}

I just checked, and it works for cpp, which also does not have _std/Array.hx implementation.
The same approach is used for String on some targets: https://github.com/HaxeFoundation/haxe/blob/development/std/js/_std/String.hx#L35
Or maybe @nicolas could advise about hl.

[Simn]

Just wanted to make sure : can we accurately make sure that the code for( idx => key in array ) will not result in any temp object being allocated in the form { key : index, value : v } ?

That depends on how well we dodge #7379, which is why I would like to specify ArrayKeyValueIterator instead of KeyValueIterator (the typedef) like in @RealyUniqueName's commit.

[elnabo]

Yeah, I just tested and it seems to works flawlessly on all targets. As there seems to be no target specific key/value iterator. It might be best to simply put it in Array. Unless there is a good reason not to, I'm going to change to that.

Maps are going to be different however.

[Simn]

Yes, maps worry me a bit because they are surely gonna hit #7379...

[Simn]

I think we should focus on Array in this PR and then merge it. Otherwise it's gonna be delayed by the other types.

[elnabo]

Using the provided commit, the k=>v operator is supported on all target (for Array) and doesn't generate a {key,value} temporary object.

[RealyUniqueName]

Still, need to implement it in the generators of eval, cpp, hl, and flash. Otherwise it won't be accessible through the structure or dynamic field access.

[RealyUniqueName]

E.g.

static var dyn:Dynamic = [1, 2, 3, 4];
static var struct:{function keyValueIterator():KeyValueIterator<Int,Int>} = [1, 2, 3, 4];

static function test() {
  trace(dyn.keyValueIterator()); //exception
  for(key => value in struct) { //exception
  }  
}

[elnabo]

I'm unsure I have the understanding to adapt all generators for this.

[RealyUniqueName]

Let's wait for @ncannasse @Simn @hughsando

[Simn]

You can add the keyValueIterator field like you did before, but then just make it construct an instance of ArrayKeyValueIterator. Check out the createInstance implementation for a reference.

Or I can look into it tomorrow.

[Simn]

I don't have time to test it right now, but something like this should work for eval:

	let keyValueIterator = vifun0 (fun vthis ->
		let ctx = get_ctx() in
		let path = key_haxe_iterators_array_key_value_iterator in
		let vit = encode_instance path in
		let fnew = get_instance_constructor ctx path null_pos in
		ignore(call_value_on vit (Lazy.force fnew) [vthis]);
		vit
	)

[elnabo]

Tried to implement it with

key_haxe_iterators_array_key_value_iterator = hash_s "haxe.iterators.ArrayKeyValueIterator"

But I get Instance prototype not found when I try to use keyValueIterator().hasNext(). The struct example is still not compiling eitherway.

[elnabo]

@Simn I tried the same as you, but my example was different. Got the same errors with your commit.

var a:Dynamic = ["a", "b", "c", "d","e"];
trace(a.keyValueIterator); // #closure
var kvi = a.keyValueIterator();
trace(kvi.hasNext()); // [0] Instance prototype not found: haxe.iterators.ArrayKeyValueIterator
trace(a.keyValueIterator().hasNext()); // [0] Instance prototype not found: haxe.iterators.ArrayKeyValueIterator

If I specify the type of kvi:haxe.iterators.ArrayKeyValueIterator<String> it works as expected.

[Simn]

I have worked on this today and just updated the initial post. The failing targets are HL, hxcpp and lua.

HL

Fails with Can't cast #hl.types.BytesKeyValueIterator_Int to #haxe.iterators.ArrayKeyValueIterator. The problem here is that we specify the return type on Array to be ArrayKeyValueIterator, not the KeyValueIterator typedef. So the types are obviously not compatible. I'm not sure how to approach this.

Lua

CI fails with bin/unit.lua:10685: attempt to index field 'arr' (a nil value) in unit.TestSpecification.testArray_unit_hx. I don't have a local environment for running lua so I can't investigate this further.

C++

Requires a patch to hxcpp to support keyValueIterator on Array. Not sure if anything else needs fixing here.

[Simn]

Sadly, I'm moving this to 4.1 because there are still some problems to sort out. The HL issue in particular worries me quite a bit.

[RealyUniqueName]

I've disabled test of anon/dynamic access to Array.keyValueIterator() for cpp target unitl HaxeFoundation/hxcpp#871 is resolved.
HL implementation is fixed, but not inlinable at all currently. Maybe @ncannasse can come up with a better solution.
Everything else seems fine.

[Simn]

@elnabo If you're still around, please update the branch.

@Aurel300 Any chance you could look into HaxeFoundation/hxcpp#871? I think that's the last part that's missing here.

[elnabo]

It should be up to date now.