HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.13k stars 653 forks source link

[python] @:coreApi removal on EReg #3458

Closed Simn closed 9 years ago

Simn commented 9 years ago

Since https://github.com/HaxeFoundation/haxe/commit/c77e8b52d7333405938fdbf387277a676ced1c53 we have a type difference issue between Int and Null<Int> in matchSub. This happens because the base version is typed as len : Int = 0 while the python version is ?len : Int. This used to not be an issue because dynamic targets would not wrap Null<T>, but they do so since above change.

I tried adjusting it but this lead to a test failure which we will have to investigate.

Simn commented 9 years ago

[insert usual rant about non-nullable optional arguments being atrocious]

ncannasse commented 9 years ago

@Simn not nullable optional arguments are necessary to preserve unboxed performances on native platforms [insert rant about people that don't care about performances] :-)

waneck commented 9 years ago

Actually I think only flash can do that without boxing. [insert rant about flash]

Simn commented 9 years ago

Except when you say "native platforms" what you really mean is "my beloved Flash target".

class Main {
    static function main() {
        test();
    }

    static function test(i:Int = 1) {
        use(i);
    }

    static function use(i:Int) { }
}

Cpp output:

Void Main_obj::test( hx::Null< int >  __o_i){
int i = __o_i.Default(1);
    HX_STACK_FRAME("Main","test",0xf1b1b087,"Main.test","Main.hx",6,0x087e5c05)
    HX_STACK_ARG(i,"i")
{
        HX_STACK_LINE(7)
        int _g = i;     HX_STACK_VAR(_g,"_g");
        HX_STACK_LINE(7)
        ::Main_obj::use(_g);
    }
return null();
}

Java output:

    public static void test(java.lang.Object i)
    {
        int __temp_i3 = ( (( i == null )) ? (((int) (1) )) : (((int) (haxe.lang.Runtime.toInt(i)) )) );
        haxe.root.Main.use(__temp_i3);
    }

And please don't give me that performance talk, I'm the only one who actually tries to improve this for non-flash targets. Here's what genc generates:

void Main__hx_known_test(int i){
    Main_use(i);
}

void Main_main(){
    Main__hx_known_test(1);
}

void Main_test(c__hx_anon_4* i){
    int _tmp0;
    if(NULL == i){
        _tmp0 = 1;
    } else {
        _tmp0 = ((int) ((i)->_hx_value));
    }
    Main__hx_known_test(_tmp0);
}

And even there I have to pre-process all field arguments and infer Null<T> on them because it simply doesn't make any sense typing-wise.

waneck commented 9 years ago

Thanks for filling up that blank for me, Simn! :P

ncannasse commented 9 years ago

Well, it's a question of language design. That's not because it's not optimized on hxcpp that it couldn't be done It makes sense to be able to have unboxed optional parameters.

For instance that could be:

int tmp = 1;
Main_test(&tmp);

void Main_test( int *_i ) {
    int i = _i ? *_i : 1;
}

And that still save an allocation.

Simn commented 9 years ago

I think I see the difference now. What I didn't consider is that with Null<Int> it would be possible to assign null to the argument identifier in the function body, e.g.:

function e(i:Null<Int> = 0) {
    i = null; // ok
}

That means it could not be "optimized" to Int in such cases.

Simn commented 9 years ago

@frabbit or @nadako: Could you check what the actual problem with EReg.matchSub is?

nadako commented 9 years ago

I don't understand the core class specification. Why len has the default value of 0? Also, @:coreApi implementors use -1 as a default value (the @:coreApi check misses that).

Speaking of python, it branches on if (len != null) which is never a case with default value.

Simn commented 9 years ago

Uhm yes, that should be -1 everywhere.

nadako commented 9 years ago

can we somehow check that by @:coreApi? iirc the problem is that default values are not part of the type, but maybe we can store them in a metadata or something.

Simn commented 9 years ago

Thanks for checking that, I sure made a lot of ruckus here for something caused by such an obvious bug. :D

I agree we should check the default values. Retrieving them should not be a problem because we have access to the cf_expr which has the TFunction.

Simn commented 9 years ago

Oh wait, what you're saying is that the core type itself doesn't have the default value... right!

Simn commented 9 years ago

Related issue: https://github.com/HaxeFoundation/haxe/issues/3451