Skip to content

Improve Falseish / Trueish for floats #787

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

Closed
wants to merge 5 commits into from
Closed

Improve Falseish / Trueish for floats #787

wants to merge 5 commits into from

Conversation

MaxGraey
Copy link
Member

Fix #770

@MaxGraey MaxGraey marked this pull request as ready for review August 23, 2019 18:10
@MaxGraey MaxGraey requested a review from dcodeIO August 23, 2019 18:19
@dcodeIO
Copy link
Member

dcodeIO commented Aug 23, 2019

One challenge here is that these functions wrap another expression with unknown state information, so the use of a local inside of the expression might conflict with the temporary local being added. Means: If tests pass currently, that's more of a coincidence because either there's no test cases causing such an issue or the functions are used in a way inside of the compiler that it cannot happen (but still might in the future).

@MaxGraey
Copy link
Member Author

Hmm. So how we can do this properly?

@MaxGraey
Copy link
Member Author

MaxGraey commented Sep 4, 2019

ping. Could you make some suggestions how we could improve this solution?

@dcodeIO
Copy link
Member

dcodeIO commented Sep 4, 2019

Hmm, thinking. If there's a local.set or .tee inside of the wrapped expression, the respective temp. local should actually be blocked still if it is used afterwards, so conflicting with inner locals shouldn't be an issue.

Remains the question if this.currentFlow is always the correct flow. Appears to be the case for makeIsFalseish since that's used in proper order on ! unaries only, but makeIsTrueish has more call sites. The scenario to look out for is that makeIsTrueish is called immediately after the expression it is going to wrap is being compiled, like any pattern of let expr = compile(...); expr = makeIsTrueish(expr) would be ok since currentFlow is guaranteed to be correct in this case. If it turns out that all call sites are safe, we should then document this on both makeIsFalseish and makeIsTrueish so future users understand the implications and don't accidentally call it at the wrong places. For instance, someone might expect that calling those is independent of context, which it isn't anymore due to adding a temp. local to the flow, which must under no circumstances overwrite a blocked local.

@MaxGraey
Copy link
Member Author

MaxGraey commented Sep 5, 2019

Great, so this changes can't broke current flow analysis?

@dcodeIO
Copy link
Member

dcodeIO commented Sep 6, 2019

Haven't checked all call sites of makeIsTrueish yet according to the scenario outlined above. Depends on those :)

@MaxGraey
Copy link
Member Author

@dcodeIO wdyt about revisit this? Will be great improve compatibility

@dcodeIO dcodeIO mentioned this pull request Nov 1, 2019
@MaxGraey
Copy link
Member Author

MaxGraey commented Nov 1, 2019

Closed for favour to #933

@MaxGraey MaxGraey closed this Nov 1, 2019
@MaxGraey MaxGraey deleted the fix-770 branch November 1, 2019 17:21
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.

Different with JS behaviour for NaN || FloatValue
2 participants