HaxeFoundation / haxe

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

Use VInlinedConstructorVariable to tag inlined but interesting variable for debug #11578

Closed yuxiaomao closed 4 months ago

yuxiaomao commented 4 months ago

This PR try to address https://github.com/vshaxe/hashlink-debugger/issues/91 (ping @ncannasse) : When performing inline constructor optimization, the original variable is lost and cannot be reconstructed at the debugger side. In this original example, pt is replaced by pt_x, pt_y (with v_kind of VInlined).

static function foo(x,y) {
    var pt = new h2d.col.Point(x,y);
    return pt.length();
}

At the same time, other inlined variable should not appear in debugger, such as z2 in the foo2() function below (z2 appear also as VInlined in foo2())

static inline function boo(z1:Float) {
    var z2 = Math.abs(z1);
    return z2 * z2;
}
static function foo2() {
    var w = boo(13.5);
    return w;
}

This PR do the following:

I have done some tests in local, and my HL debugger can see those variables that I'd like to have (it can't parse the value correctly with the "." but I would like to validate the haxe change in a first place).

Simn commented 4 months ago

Interesting failure in tests/optimization:

src/Test.hx:125: lines 125-127 : Warning : Expected: var _g_set_amount = 10;var _g_current = 0;while(_g_current++ < 10) {var i = null;}
src/Test.hx:125: lines 125-127 : Warning : Actual  : var inlCollectionIterator_set_amount = 10;var _g_current = 0;while(_g_current++ < 10) {var i = null;}

I think the new version is actually better because the variable name is more descriptive. I'm not sure why this changed here though.

yuxiaomao commented 4 months ago

Hum, I think it is because I alloc VInlinedConstructorVariable instead of VInlined, so during get_pretty_name it get the VGenerated name (which is cl_path) instead of VInlined name (which is just fname)?

Simn commented 4 months ago

I'm fine with merging this, we can deal with the is_gen_local question later because I won't be able to help much with that anyway. Unless there's anything else you wanted to do here before merging?

yuxiaomao commented 4 months ago

Just discussed with @ncannasse , I'll rename is_gen_local and change argument of VInlinedConstructorVariable from string to string list. Please wait next commit.

Simn commented 4 months ago

I wanted to suggest a string list too but I'm not sure if we actually care about the individual parts of it. If so then yes, that's better indeed.