Groups keyboard shortcuts have been updated
Dismiss
See shortcuts
[XL] Change in dart/sdk[main]: Add documentation, tests and better parameter names to Float32x4.
0 views
Skip to first unread message
Alexander Markov (Gerrit)
Mar 13, 2025, 10:42:07 AMMar 13
to Lasse Nielsen, Alexander Markov, Stephen Adams, Commit Queue, dart-dc-te...@google.com, dart2js-te...@google.com, dart2wasm-t...@google.com, rev...@dartlang.org
Attention needed from Lasse Nielsen and Stephen Adams
Alexander Markov voted and added 5 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
5 comments
File sdk/lib/_internal/vm/lib/typed_data_patch.dart
Line 3800, Patchset 10 (Parent):
_throwIfNull(v, 'v');Alexander Markov . unresolved
Consider removing `_throwIfNull` entirely along with all its uses in this file.
File sdk/lib/typed_data/typed_data.dart
Line 2687, Patchset 10 (Latest):
/// and [Float32x4.w] value, which is the 32-bit floating point positive zeroAlexander Markov . unresolved
Nit: do we really need to emphasize this here and below? It doesn't add any more clarity than simply saying "zero".
Line 2698, Patchset 10 (Latest):
/// 32-bit integers and 32-bit floating point numbers. If both use the same
/// endianness (most platforms do), the result is predictable. If a CPU
/// has different endianness for integers and floating point numbers,
/// the result will differ.Alexander Markov . unresolved
Nit: why having this comment? It does not clarify anything: the result is predictable anyway, for any given platform. And it is not clear from what it will differ.
Line 2755, Patchset 10 (Latest):
/// to a result which is true of the former is *less than*Alexander Markov . unresolved
The result is -1 if less than and 0 otherwise.
(also below for other comparisons)
Line 2857, Patchset 10 (Latest):
/// Lane-wise clamp clamp to a range.Alexander Markov . unresolved
clamp
Related details
Attention is currently required from:
- Lasse Nielsen
- Stephen Adams
Submit Requirements:
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Mar 13, 2025, 11:45:41 AMMar 13
to Alexander Markov, Stephen Adams, Commit Queue, dart-dc-te...@google.com, dart2js-te...@google.com, dart2wasm-t...@google.com, rev...@dartlang.org
Attention needed from Stephen Adams
Lasse Nielsen added 5 comments![]( "Open in Gerrit")
File sdk/lib/_internal/vm/lib/typed_data_patch.dart
Line 3800, Patchset 10 (Parent):
_throwIfNull(v, 'v');Alexander Markov . resolved
Consider removing `_throwIfNull` entirely along with all its uses in this file.
Lasse Nielsen
Gladly 😊
File sdk/lib/typed_data/typed_data.dart
Line 2687, Patchset 10:
/// and [Float32x4.w] value, which is the 32-bit floating point positive zeroAlexander Markov . resolved
Nit: do we really need to emphasize this here and below? It doesn't add any more clarity than simply saying "zero".
Lasse Nielsen
Probably not. If we say "negative zero" when it's that, then "zero" should mean positive zero.
Line 2698, Patchset 10:
/// 32-bit integers and 32-bit floating point numbers. If both use the same
/// endianness (most platforms do), the result is predictable. If a CPU
/// has different endianness for integers and floating point numbers,
/// the result will differ.Alexander Markov . resolved
Nit: why having this comment? It does not clarify anything: the result is predictable anyway, for any given platform. And it is not clear from what it will differ.
Lasse Nielsen
It's deliberately vague because I don't know what could cause this to fail.
I have heard of platforms with different (or weird) endianness for doubles, but it's probably not the same for 32-bit floats.
I'll remove it.
Line 2755, Patchset 10:
/// to a result which is true of the former is *less than*Alexander Markov . resolved
The result is -1 if less than and 0 otherwise.
(also below for other comparisons)
Lasse Nielsen
I'm trying (but not sure I'm succeeding) to separate the truth value from the way it's represented.
Maybe it's just confusing. Will rewrite.
Line 2857, Patchset 10:
/// Lane-wise clamp clamp to a range.Alexander Markov . resolved
Lasse Nielsenclamp
Done
Related details
Attention is currently required from:
- Stephen Adams
Submit Requirements:
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Mar 14, 2025, 9:22:36 AMMar 14
to Alexander Markov, Stephen Adams, Commit Queue, dart-dc-te...@google.com, dart2js-te...@google.com, dart2wasm-t...@google.com, rev...@dartlang.org
Attention needed from Stephen Adams
Lasse Nielsen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Stephen Adams
Submit Requirements:
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Mar 14, 2025, 9:22:54 AMMar 14
to Lasse Nielsen, Alexander Markov, Stephen Adams, dart-dc-te...@google.com, dart2js-te...@google.com, dart2wasm-t...@google.com, rev...@dartlang.org
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: sdk/lib/_internal/vm/lib/typed_data_patch.dart
Insertions: 0, Deletions: 36.
@@ -3777,10 +3777,6 @@
@patch
@pragma("vm:prefer-inline")
factory Float32x4(double x, double y, double z, double w) {
- _throwIfNull(x, 'x');
- _throwIfNull(y, 'y');
- _throwIfNull(z, 'z');
- _throwIfNull(w, 'w');
return _Float32x4FromDoubles(x, y, z, w);
}
@@ -3911,7 +3907,6 @@
@pragma("vm:prefer-inline")
Float32x4 withX(double x) {
- _throwIfNull(x, 'x');
return _withX(x);
}
@@ -3922,7 +3917,6 @@
@pragma("vm:prefer-inline")
Float32x4 withY(double y) {
- _throwIfNull(y, 'y');
return _withY(y);
}
@@ -3933,7 +3927,6 @@
@pragma("vm:prefer-inline")
Float32x4 withZ(double z) {
- _throwIfNull(z, 'z');
return _withZ(z);
}
@@ -3944,7 +3937,6 @@
@pragma("vm:prefer-inline")
Float32x4 withW(double w) {
- _throwIfNull(w, 'w');
return _withW(w);
}
@@ -3981,10 +3973,6 @@
@patch
@pragma("vm:prefer-inline")
factory Int32x4(int x, int y, int z, int w) {
- _throwIfNull(x, 'x');
- _throwIfNull(y, 'y');
- _throwIfNull(z, 'z');
- _throwIfNull(w, 'w');
return _Int32x4FromInts(x, y, z, w);
}
@@ -3996,10 +3984,6 @@
@patch
@pragma("vm:prefer-inline")
factory Int32x4.bool(bool x, bool y, bool z, bool w) {
- _throwIfNull(x, 'x');
- _throwIfNull(y, 'y');
- _throwIfNull(z, 'z');
- _throwIfNull(w, 'w');
return _Int32x4FromBools(x, y, z, w);
}
@@ -4050,7 +4034,6 @@
@pragma("vm:prefer-inline")
Int32x4 withX(int x) {
- _throwIfNull(x, 'x');
return _withX(x);
}
@@ -4060,7 +4043,6 @@
@pragma("vm:prefer-inline")
Int32x4 withY(int y) {
- _throwIfNull(y, 'y');
return _withY(y);
}
@@ -4070,7 +4052,6 @@
@pragma("vm:prefer-inline")
Int32x4 withZ(int z) {
- _throwIfNull(z, 'z');
return _withZ(z);
}
@@ -4080,7 +4061,6 @@
@pragma("vm:prefer-inline")
Int32x4 withW(int w) {
- _throwIfNull(w, 'w');
return _withW(w);
}
@@ -4107,7 +4087,6 @@
@pragma("vm:prefer-inline", _Int32x4)
Int32x4 withFlagX(bool x) {
- _throwIfNull(x, 'x');
return _withFlagX(x);
}
@@ -4118,7 +4097,6 @@
@pragma("vm:prefer-inline", _Int32x4)
Int32x4 withFlagY(bool y) {
- _throwIfNull(y, 'y');
return _withFlagY(y);
}
@@ -4129,7 +4107,6 @@
@pragma("vm:prefer-inline", _Int32x4)
Int32x4 withFlagZ(bool z) {
- _throwIfNull(z, 'z');
return _withFlagZ(z);
}
@@ -4140,7 +4117,6 @@
@pragma("vm:prefer-inline", _Int32x4)
Int32x4 withFlagW(bool w) {
- _throwIfNull(w, 'w');
return _withFlagW(w);
}
@@ -4161,8 +4137,6 @@
@patch
@pragma("vm:prefer-inline")
factory Float64x2(double x, double y) {
- _throwIfNull(x, 'x');
- _throwIfNull(y, 'y');
return _Float64x2FromDoubles(x, y);
}
@@ -4174,7 +4148,6 @@
@patch
@pragma("vm:prefer-inline")
factory Float64x2.splat(double v) {
- _throwIfNull(v, 'v');
return _Float64x2Splat(v);
}
@@ -4241,7 +4214,6 @@
@pragma("vm:prefer-inline")
Float64x2 withX(double x) {
- _throwIfNull(x, 'x');
return _withX(x);
}
@@ -4252,7 +4224,6 @@
@pragma("vm:prefer-inline")
Float64x2 withY(double y) {
- _throwIfNull(y, 'y');
return _withY(y);
}
@@ -5432,13 +5403,6 @@
return value;
}
-@pragma("vm:prefer-inline")
-void _throwIfNull(val, String name) {
- if (val == null) {
- throw ArgumentError.notNull(name);
- }
-}
-
// Length should be non-negative.
@pragma("vm:recognized", "other")
@pragma("vm:prefer-inline")
```
```
The name of the file: sdk/lib/typed_data/typed_data.dart
Insertions: 51, Deletions: 44.
@@ -2684,8 +2684,7 @@
/// Creates a `Float32x4` with all values being zero.
///
/// The created value has the same [Float32x4.x], [Float32x4.y], [Float32x4.z]
- /// and [Float32x4.w] value, which is the 32-bit floating point positive zero
- /// value.
+ /// and [Float32x4.w] value, which is the 32-bit floating point zero value.
external factory Float32x4.zero();
/// Creates a `Float32x4` with 32-bit float values from the provided bits.
@@ -2695,10 +2694,7 @@
/// of the bit-representations of the corresponding lanes of [bits].
///
/// The conversion is performed using the *platform endianness* for both
- /// 32-bit integers and 32-bit floating point numbers. If both use the same
- /// endianness (most platforms do), the result is predictable. If a CPU
- /// has different endianness for integers and floating point numbers,
- /// the result will differ.
+ /// 32-bit integers and 32-bit floating point numbers.
external factory Float32x4.fromInt32x4Bits(Int32x4 bits);
/// Creates a `Float32x4` with its [x] and [y] lanes set to values from [xy].
@@ -2706,8 +2702,7 @@
/// The created value has [Float32x4.x] and [Float32x4.y] values
/// which are the conversions of the [Float64x2.x] and [Float64x2.y] lanes
/// of [xy] to 32-bit floating point values.
- /// The [Float32x4.z] and [Float32x4.w] lanes are set to the positive zero
- /// value.
+ /// The [Float32x4.z] and [Float32x4.w] lanes are set to the zero value.
external factory Float32x4.fromFloat64x2(Float64x2 xy);
/// Lane-wise addition.
@@ -2750,86 +2745,98 @@
/// Lane-wise less-than comparison.
///
- /// Compares the value of each lane of this value
+ /// Compares the 32-bit floating point value of each lane of this
/// to the value of the corresponding lane of [other],
- /// to a result which is true of the former is *less than*
- /// the latter as 32-bit floating-point values.
+ /// using 32-bit floating point comparison.
/// _For floating point comparisons, a comparison with a NaN value is
/// always false, and -0.0 (negative zero) is considered equal to 0.0
/// (positive zero), and not less strictly than it._
+ /// The result for a lane is a 32-bit signed integer which is -1
+ /// (all bits set) if the value from this object is *less than*
+ /// the value from [other], and the result is 0 (all bits cleared) if not,
+ /// including if either value is a NaN value.
///
- /// Returns a representation of the comparison results for each lane
- /// with a 32-bit integer value if -1 for true and 0 for false.
+ /// Returns four values that are always either 0 or -1.
Int32x4 lessThan(Float32x4 other);
/// Lane-wise less-than-or-equal comparison.
///
- /// Compares the value of each lane of this value
+ /// Compares the 32-bit floating point value of each lane of this
/// to the value of the corresponding lane of [other],
- /// to a result which is true of the former is *less than or equal to*
- /// the latter as 32-bit floating-point values.
+ /// using 32-bit floating point comparison.
/// _For floating point comparisons, a comparison with a NaN value is
/// always false, and -0.0 (negative zero) is considered equal to 0.0
- /// (positive zero), and not strictly less than it._
+ /// (positive zero), and not less strictly than it._
+ /// The result for a lane is a 32-bit signed integer which is -1
+ /// (all bits set) if the value from this object is *less than or equal to*
+ /// the value from [other], and the result is 0 (all bits cleared) if not,
+ /// including if either value is a NaN value.
///
- /// Returns a representation of the comparison results for each lane
- /// with a 32-bit integer value if -1 for true and 0 for false.
+ /// Returns four values that are always either 0 or -1.
Int32x4 lessThanOrEqual(Float32x4 other);
/// Lane-wise greater-than comparison.
///
- /// Compares the value of each lane of this value
+ /// Compares the 32-bit floating point value of each lane of this
/// to the value of the corresponding lane of [other],
- /// to a result which is true of the former is *greater than*
- /// the latter as 32-bit floating-point values.
+ /// using 32-bit floating point comparison.
/// _For floating point comparisons, a comparison with a NaN value is
/// always false, and -0.0 (negative zero) is considered equal to 0.0
- /// (positive zero), and not strictly less than it._
+ /// (positive zero), and not less strictly than it._
+ /// The result for a lane is a 32-bit signed integer which is -1
+ /// (all bits set) if the value from this object is *greater than*
+ /// the value from [other], and the result is 0 (all bits cleared) if not,
+ /// including if either value is a NaN value.
///
- /// Returns a representation of the comparison results for each lane
- /// with a 32-bit integer value if -1 for true and 0 for false.
+ /// Returns four values that are always either 0 or -1.
Int32x4 greaterThan(Float32x4 other);
/// Lane-wise greater-than-or-equal comparison.
///
- /// Compares the value of each lane of this value
+ /// Compares the 32-bit floating point value of each lane of this
/// to the value of the corresponding lane of [other],
- /// to a result which is true of the former is *greater than or equal to*
- /// the latter as 32-bit floating-point values.
+ /// using 32-bit floating point comparison.
/// _For floating point comparisons, a comparison with a NaN value is
/// always false, and -0.0 (negative zero) is considered equal to 0.0
- /// (positive zero), and not strictly less than it._
+ /// (positive zero), and not less strictly than it._
+ /// The result for a lane is a 32-bit signed integer which is -1
+ /// (all bits set) if the value from this object is *greater than or equal to*
+ /// the value from [other], and the result is 0 (all bits cleared) if not,
+ /// including if either value is a NaN value.
///
- /// Returns a representation of the comparison results for each lane
- /// with a 32-bit integer value if -1 for true and 0 for false.
+ /// Returns four values that are always either 0 or -1.
Int32x4 greaterThanOrEqual(Float32x4 other);
/// Lane-wise equals comparison.
///
- /// Compares the value of each lane of this value
+ /// Compares the 32-bit floating point value of each lane of this
/// to the value of the corresponding lane of [other],
- /// to a result which is true of the former is *equal to*
- /// the latter as 32-bit floating-point values.
+ /// using 32-bit floating point comparison.
/// _For floating point comparisons, a comparison with a NaN value is
/// always false, and -0.0 (negative zero) is considered equal to 0.0
- /// (positive zero), and not strictly less than it._
+ /// (positive zero), and not less strictly than it._
+ /// The result for a lane is a 32-bit signed integer which is -1
+ /// (all bits set) if the value from this object is *equal to*
+ /// the value from [other], and the result is 0 (all bits cleared) if not,
+ /// including if either value is a NaN value.
///
- /// Returns a representation of the comparison results for each lane
- /// with a 32-bit integer value if -1 for true and 0 for false.
+ /// Returns four values that are always either 0 or -1.
Int32x4 equal(Float32x4 other);
/// Lane-wise not-equals comparison.
///
- /// Compares the value of each lane of this value
+ /// Compares the 32-bit floating point value of each lane of this
/// to the value of the corresponding lane of [other],
- /// to a result which is true of the former is *not equal to*
- /// the latter as 32-bit floating-point values.
+ /// using 32-bit floating point comparison.
/// _For floating point comparisons, a comparison with a NaN value is
/// always false, and -0.0 (negative zero) is considered equal to 0.0
- /// (positive zero), and not strictly less than it._
+ /// (positive zero), and not less strictly than it._
+ /// The result for a lane is a 32-bit signed integer which is -1
+ /// (all bits set) if the value from this object is *not equal to*
+ /// the value from [other], and the result is 0 (all bits cleared) if not,
+ /// including if either value is a NaN value.
///
- /// Returns a representation of the comparison results for each lane
- /// with a 32-bit integer value if -1 for true and 0 for false.
+ /// Returns four values that are always either 0 or -1.
Int32x4 notEqual(Float32x4 other);
/// Lane-wise multiplication by [scale].
@@ -2854,7 +2861,7 @@
/// Returns the result for each lane.
Float32x4 abs();
- /// Lane-wise clamp clamp to a range.
+ /// Lane-wise clamp to a range.
///
/// Clamps the value of each lane to a minimum value
/// of the corresponding lane of [lowerLimit]
```
Change information
Commit message:
Add documentation, tests and better parameter names to Float32x4.
The same should happen for `Int32x4` and `Float64x2`,
but this is a start.
The existing class was basically uncommented and you had
to guess or explore to find out what, fx, the returned
values for the relational operators means.
I've tried to discover and document the current behavior,
but I had to fall back on "it's implementation specific"
in some places where platforms behave differently.
(For example it seems VM on Mac treats `.reciprocalSqrt()`
as doing `.sqrt().reciprocal()` where the other platforms do
`.reciprocal().sqrt()`, which makes a difference for exactly
one value.)
CoreLibraryReviewExempt: Doc and parameter name changes only.
Tested: Added more tests to float32x4_test.dart.
Change-Id: I3f8804f842d8a5d95e1d378a7edb9438a2f3ed2b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/415000
Commit-Queue: Lasse Nielsen <l...@google.com>
Reviewed-by: Alexander Markov <alexm...@google.com>
Files:
- M sdk/lib/_internal/js_dev_runtime/patch/typed_data_patch.dart
- M sdk/lib/_internal/js_dev_runtime/private/native_typed_data.dart
- M sdk/lib/_internal/js_runtime/lib/native_typed_data.dart
- M sdk/lib/_internal/js_runtime/lib/typed_data_patch.dart
- M sdk/lib/_internal/vm/lib/typed_data_patch.dart
- M sdk/lib/_internal/wasm/lib/simd.dart
- M sdk/lib/_internal/wasm/lib/simd_patch.dart
- M sdk/lib/typed_data/typed_data.dart
- M tests/lib/typed_data/float32x4_test.dart
- M tests/lib/typed_data/int32x4_test.dart
Change size: XL
Delta: 10 files changed, 2296 insertions(+), 658 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Alexander Markov
Core-Library-Review: Code-Review+1 by Alexander Markov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Mar 14, 2025, 2:15:17 PMMar 14
to Commit Queue, Lasse Nielsen, Alexander Aprelev, Alexander Markov, Stephen Adams, dart-dc-te...@google.com, dart2js-te...@google.com, dart2wasm-t...@google.com, rev...@dartlang.org
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 13 (Latest):
Alexander Aprelev . resolved
android https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8720415354162781249/+/u/test_results/new_test_failures__logs_ doesn't seem to be happy with new test
```
Expect.equals(expected: <0>, actual: <-1>) fails.
#0 Expect._fail (package:expect/expect.dart:887)
#1 Expect._failNotEqual (package:expect/expect.dart:174)
#2 Expect.equals (package:expect/expect.dart:157)
#3 testComparison (file:///b/swarming/w/ir/tests/lib/typed_data/float32x4_test.dart:160)
#4 main (file:///b/swarming/w/ir/tests/lib/typed_data/float32x4_test.dart:651)
#5 _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:315)
#6 _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:194)
ExitCode: 255
```
Related details
Attention set is empty
Submit Requirements:
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages