You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Before this PR this will create unnecessary cast to f64, Math.floor and downcast to i32. With this PR this code now equivalent to x / y without all this unnecessary work due to Math.floor, Math.trunc and etc accept integers and pass through it "as is".
Now following code will generate a compilation error:
constcos3=Math.cos(3);// -> ERROR: Math.cos accept only f32 or f64 types// Solution: it may be fixed as Math.cos(3.0)constx: i32=123;constsqr=Math.sqrt(x);// -> ERROR: Math.sqrt accept only f32 or f64 types// Solution: Math.sqrt(x as f64)
This is the most painful caveat, which can lead to many breaking changes in some code.
Perhaps we could add new generic builtin helper which forces parameter type to float. Something like:
// builtin will be equivalent to this:typeOnlyFloat<T>=Textendsf32|f64 ? T : f64functionsqrt<Textendsi32|i64|f32|f64>(x: T): OnlyFloat<T>{if(isInteger<T>()){returnbuiltin_sqrt<f64>(xasf64)asOnlyFloat<T>;}if(isFloat<T>()){returnbuiltin_sqrt<T>(x)asOnlyFloat<T>;}}// now we can use integer inputs which will forced to f64 for outputs but safely preserve f32constsqrt3=sqrt(3);// infer as sqrt<i32>(x) -> f64
So far, Math is quite intentionally identical to JS Math, also because the implementations can be switched with --use. Making one generic and the other not seems problematic in this regard. Similarly, this might break portable code relying on both being identical. The mentioned caveat that integer literals will be inferred as i32, which sometimes might either not work or produce unexpected results in the absence of a type parameter, seems quite unfortunate as well. Are you sure this is actually viable?
abs, floor, ceil, round etc will be work properly with integers even in JSMath due to i32 -> f64 -> i32 is invariant. But u32 -> f64 -> u32 or i64 -> f64 -> i64 is not invariant, so this may be a problem. However, I don't think ppl will use that functions for such types. Also, I'm going to add some compiler warnings for integer paths for according methos.
Has this been solved? It has been almost 3 years since the last commit. Also maybe casting the type to f64 could make sense in this case?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR we're doing several breaking changes:
std/util/mathNativeMath#scalebnNativeMath#remNativeMath#mod(but not tests)NativeMathfcompletelyMath.random(forf64type)Caveats:
Before this PR this will create unnecessary cast to f64,
Math.floorand downcast toi32. With this PR this code now equivalent tox / ywithout all this unnecessary work due toMath.floor,Math.truncand etc accept integers and pass through it "as is".This is the most painful caveat, which can lead to many breaking changes in some code.
Perhaps we could add new generic builtin helper which forces parameter type to float. Something like:
Thoughts?