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

Don't ignore default values of arguments of inlined functions #8397

Conversation

RealyUniqueName
Copy link
Member

Fixes #7032

@RealyUniqueName RealyUniqueName added this to the Release 4.0 milestone Jun 10, 2019
@RealyUniqueName RealyUniqueName requested a review from Simn June 10, 2019 09:13
@skial skial mentioned this pull request Jun 11, 2019
1 task
@Simn
Copy link
Member

Simn commented Jun 19, 2019

This doesn't seem quite right because we now handle default values in two places. This PR would introduce unwanted branching in cases where we have all the static information, e.g. if we're calling your def2 directly.

Please make sure to check the output without -D analyzer-optimize to make sure this doesn't happen. The branching should only occur if we're calling the function with a statically unknown value.

@RealyUniqueName
Copy link
Member Author

RealyUniqueName commented Jun 19, 2019

Actually, we don't handle default values like that anywhere in the compiler. Generators have to do that manually. And this branching is only inserted on the call site. Original function body is not changed.

Please make sure to check the output without -D analyzer-optimize to make sure this doesn't happen. The branching should only occur if we're calling the function with a statically unknown value.

Here is the generated js:

// Generated by Haxe 4.0.0-rc.3
(function ($global) { "use strict";
var Main = function() { };
Main.main = function() {
	console.log("src/Main.hx:3:",10);  // This is for `trace(def2(null))` or `trace(def2())`
	console.log("src/Main.hx:4:",Main.def(null));
};
Main.def = function(p) {
	var p1 = p;
	if(p == null) {
		p1 = 10;
	}
	return p1;
};
Main.main();
})({});

@Simn
Copy link
Member

Simn commented Jun 19, 2019

class Main {
	static function main() {
		trace(f());
	}

	static inline function f(p = 10) {
		return p;
	}
}

Compiled to JS without -D analyzer-optimize:

Main.main = function() {
	var p = 10;
	if(p == null) {
		p = 10;
	}
	console.log("source/Main.hx:3:",p);
};

@RealyUniqueName
Copy link
Member Author

Sorry, I run it with analyzer-optimize

@RealyUniqueName
Copy link
Member Author

What would be the correct fix then?
Because if I have a code like this:

var p = null;
trace(f(p));

does it count for p being statucally known? Or do we need an analyzer pass prior to inlining to be able to fix the issue?

@Simn
Copy link
Member

Simn commented Jun 25, 2019

In that case we have to accept the branching, nothing we can do about it. The analyzer will clean this up where possible and it's fair to require its usage in these situations.

Aurel300 added a commit to Aurel300/hxcpp that referenced this pull request Jun 26, 2019
@Simn Simn closed this in 4294af1 Jul 1, 2019
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.

optional argument default value lost when inlining a function call
2 participants