Vector.fill

[RblSb]

Closes https://github.com/HaxeFoundation/haxe/issues/8040 This is only 2-4 times faster on js (node/chrome), btw.

[Simn]

I'd expect this to be an instance method.

[RblSb]

You cannot generate filled non-nullable Vector in instance after new Vector(), so there is both (see https://github.com/HaxeFoundation/haxe/issues/8040#issuecomment-495991325).

[Simn]

Oh there are two functions... that is some rather confusing naming. I wonder if we can just support overloading the constructor for this. Not sure if that just works already.

[Gama11]

that is some rather confusing naming.

Is it? The naming makes sense to me.

[tobil4sk]

If the constructor route isn't possible, I think something like createFilled() would make the purpose more explicit.

[sebthom]

I'd like to see this improvement too.

@RblSb could you please update your PR and change the name of the factory method from filled to createFilled? I think then we are closer to a consensus.

[sebthom]

@RblSb Thanks!

@Gama11 @Simn @tobil4sk now that the method is properly named. are there still any objections? if not can anyone of you please merge this? My hands are tied :-)

[kLabz]

CI is not OK yet

/home/runner/work/haxe/haxe/std/hl/_std/haxe/ds/Vector.hx:28: lines 28-90 : Missing field createFilled required by core type

[sebthom]

@RblSb The tests are green now but I wonder if the same would need to be done for https://github.com/HaxeFoundation/haxe/blob/development/std/php/_std/haxe/ds/Vector.hx too

[tobil4sk]

I wonder if we can just support overloading the constructor for this. Not sure if that just works already.

Another option would be to just have the defaultValue as a new optional parameter to the constructor. Would there be any problems with that?

[RblSb]

@tobil4sk there is a problem https://github.com/HaxeFoundation/haxe/issues/10986

[tobil4sk]

~~I mean with a single constructor like this, like proposed in the original issue~~ (nvm, I just saw https://github.com/HaxeFoundation/haxe/pull/9478):

public inline function new(length:Int, ?defaultValue:T) {
	this = new java.NativeArray(length);
	if (defaultValue != null) {
		// fill
	}
}

~~Since defaultValue is optional, the same constructor could still be called without the defaultValue. Is there any reason why something like this couldn't work?~~

I wonder if the same would need to be done for https://github.com/HaxeFoundation/haxe/blob/development/std/php/_std/haxe/ds/Vector.hx too

The php implementation is missing @:coreApi meta, could be why the missing fields didn't cause an error.

[tobil4sk]

@tobil4sk there is a problem https://github.com/HaxeFoundation/haxe/issues/10986

This is only a problem during development, no? Once the Vector constructor overload is complete, the user won't be changing the Vector class so they won't be affected.

[RblSb]

@tobil4sk there is a problem #10986

This is only a problem during development, no? Once the Vector constructor overload is complete, the user won't be changing the Vector class so they won't be affected.

Yes, but this bug reproduces even if you change any comment in something like std.StringTools, and maybe even with complex user-space code, macros, etc. I would like new Vector(5, 1) little more too, but it just breaks jvm target for now.

[RblSb]

Vector.get(index): If index is negative or exceeds this.length, the result is unspecified.

this.length - 1 is probably more correct doc? (or "equals this.length or exceeds it")

[tobil4sk]

Yes, but this bug reproduces even if you change any comment in something like std.StringTools

So to clarify, adding the extra constructor results in future changes to other classes breaking with the same error? In that case I think it's worth waiting for the issue to be investigated to see if we can use the overload constructor version.

Also, for php you could use the native array_fill method, for example like this: https://github.com/tobil4sk/haxec/commit/36dafd80a09905d06d39b0095e3ac390704b328f.

[RblSb]

Should be good for merge

[Simn]

I'm lowkey nervous about using this quite new overload mechanism on something important like Vector, but I guess it's a good thing because we'll know if it goes wrong very quickly. Will merge this once CI had some time to breathe.