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

update std.math functions to widen instead of returning errors #4481

Open
andrewrk opened this issue Feb 16, 2020 · 2 comments
Open

update std.math functions to widen instead of returning errors #4481

andrewrk opened this issue Feb 16, 2020 · 2 comments
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@andrewrk
Copy link
Member

I propose to update:

  • std.math.mul
  • std.math.add
  • std.math.sub
  • etc

To not have error.Overflow error. Instead something like this:

diff --git a/lib/std/math.zig b/lib/std/math.zig
index 5885964c3..3aa43832c 100644
--- a/lib/std/math.zig
    b/lib/std/math.zig
@@ -322,11  322,6 @@ test "math.clamp" {
     testing.expect(std.math.clamp(@as(i32, 8), @as(i32, 7), @as(i32, -4)) == 7);
 }
 
-pub fn mul(comptime T: type, a: T, b: T) (error{Overflow}!T) {
-    var answer: T = undefined;
-    return if (@mulWithOverflow(T, a, b, &answer)) error.Overflow else answer;
-}
-
 pub fn add(comptime T: type, a: T, b: T) (error{Overflow}!T) {
     var answer: T = undefined;
     return if (@addWithOverflow(T, a, b, &answer)) error.Overflow else answer;
@@ -949,15  944,43 @@ test "max value type" {
     testing.expect(x == 2147483647);
 }
 
-pub fn mulWide(comptime T: type, a: T, b: T) @IntType(T.is_signed, T.bit_count * 2) {
-    const ResultInt = @IntType(T.is_signed, T.bit_count * 2);
-    return @as(ResultInt, a) * @as(ResultInt, b);
 /// Multiply two integers together, without possibility of overflow.
 /// The return type will be just wide enough to be guaranteed to store the result.
 /// This function is safe and does not assert anything.
 pub fn mul(a: var, b: var) Mul(@TypeOf(a), @TypeOf(b)) {
     const ResultInt = Mul(@TypeOf(a), @TypeOf(b));
     // TODO implementation
 }
 
-test "math.mulWide" {
-    testing.expect(mulWide(u8, 5, 5) == 25);
-    testing.expect(mulWide(i8, 5, -5) == -25);
-    testing.expect(mulWide(u8, 100, 100) == 10000);
 test "mul" {
     {
         var a: u8 = 5;
         var b: u8 = 5;
         const result = mul(a, b);
         testing.expect(result == 25);
         testing.expect(@TypeOf(result) == u16);
     }
     {
         var a: u8 = 5;
         var b: i8 = -5;
         const result = mul(a, b);
         testing.expect(result == -25);
         testing.expect(@TypeOf(result) == i16);
     }
     {
         var a: u8 = 100;
         var b: u8 = 100;
         const result = mul(a, b);
         testing.expect(result == 10000);
         testing.expect(@TypeOf(result) == u16);
     }
     {
         var a: u8 = 100;
         var b: u8 = 100;
         const result = mul(@as(u8, 9), @as(i8, 100));
         testing.expect(result == 900);
         testing.expect(@TypeOf(result) == comptime_int);
     }
 }
 
 /// See also `CompareOperator`.

And then std.math.cast can be used to cast the value to a smaller integer, or return error.Overflow.

@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Feb 16, 2020
@andrewrk andrewrk added this to the 0.7.0 milestone Feb 16, 2020
@andrewrk
Copy link
Member Author

This proposal harmonizes with #3806.

@squeek502
Copy link
Collaborator

squeek502 commented Feb 20, 2020

Related: std.math.ceilPowerOfTwo (can return error.Overflow) and std.math.ceilPowerOfTwoPromote (can't fail, returns a widened type):

zig/lib/std/math.zig

Lines 792 to 817 in 1483ae3

/// Returns the next power of two (if the value is not already a power of two).
/// Only unsigned integers can be used. Zero is not an allowed input.
/// Result is a type with 1 more bit than the input type.
pub fn ceilPowerOfTwoPromote(comptime T: type, value: T) @IntType(T.is_signed, T.bit_count 1) {
comptime assert(@typeId(T) == builtin.TypeId.Int);
comptime assert(!T.is_signed);
assert(value != 0);
comptime const PromotedType = @IntType(T.is_signed, T.bit_count 1);
comptime const shiftType = std.math.Log2Int(PromotedType);
return @as(PromotedType, 1) << @intCast(shiftType, T.bit_count - @clz(T, value - 1));
}
/// Returns the next power of two (if the value is not already a power of two).
/// Only unsigned integers can be used. Zero is not an allowed input.
/// If the value doesn't fit, returns an error.
pub fn ceilPowerOfTwo(comptime T: type, value: T) (error{Overflow}!T) {
comptime assert(@typeId(T) == builtin.TypeId.Int);
comptime assert(!T.is_signed);
comptime const PromotedType = @IntType(T.is_signed, T.bit_count 1);
comptime const overflowBit = @as(PromotedType, 1) << T.bit_count;
var x = ceilPowerOfTwoPromote(T, value);
if (overflowBit & x != 0) {
return error.Overflow;
}
return @intCast(T, x);
}

There was also a tiny bit of discussion of handling overflow in the PR for those two functions: #2617

@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 27, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 23, 2021
@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Apr 16, 2022
@andrewrk andrewrk modified the milestones: 0.11.0, 0.12.0 Apr 9, 2023
@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

2 participants