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

remove builtin @minValue and @maxValue and replace with stdlib implementation; #1476

Closed
wants to merge 21 commits into from

Conversation

Projects
None yet
5 participants
@kristate
Copy link
Contributor

commented Sep 5, 2018

fixes #1466

  • Remove builtin @minValue and @maxValue from stage1
  • Replace all instances of @minValue and @maxValue with std.math.minInt and std.math.maxInt
  • update / include tests
i64 => -9223372036854775808,
i128 => -170141183460469231731687303715884105728,
else => {
//Calculate for Integers that we do not have cached

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

I don't think there is any reason for this explicit caching since the compiler already does this.

This comment has been minimized.

Copy link
@kristate

kristate Sep 5, 2018

Author Contributor

What does "the compiler already does this" mean here? we are in comptime, so obviously we are not caching at runtime, but I thought that even at comptime, having a switch for common types could speed-up compile times.

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

Well, each instantiation of min/maxInt only gets its body comptime evaluated once and then cached. This is how types such as ArrayList(u8) is the same type each time it is called. Isn't the point of comptime stuff that we can avoid hardcoding our comptime generated tables and such? :)

This comment has been minimized.

Copy link
@kristate

kristate Sep 6, 2018

Author Contributor

@Hejsil I didn't realize that all return values of comptime were cached. Is that defined somewhere in the documentation? I was aware of using comptime to create lookup tables, etc for crc32 [0].

I did a test calling std.math.maxInt(u32) repeatedly and it did cache, so I will remove the switch.

return switch (@typeId(T)) {
// - (1 << (T.bit_count - 1))
TypeId.Int, TypeId.ComptimeInt => if (T.is_signed) -(1 << (T.bit_count - 1)) else 0,
else => @compileError("minValue not implemented for " ++ @typeName(T)),

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

Should this function also find the max value of an enum?
Use case: User made enum arrays

This comment has been minimized.

Copy link
@andrewrk

andrewrk Sep 5, 2018

Member

I'm thinking it should probably just be std.maxInt and std.minInt. max/min values of enums can be separate functions.

This comment has been minimized.

Copy link
@kristate

kristate Sep 5, 2018

Author Contributor

Okay, I will change the function name.

@@ -380,7 +456,7 @@ pub fn absInt(x: var) !@typeOf(x) {
comptime assert(@typeId(T) == builtin.TypeId.Int); // must pass an integer to absInt
comptime assert(T.is_signed); // must pass a signed integer to absInt

if (x == @minValue(@typeOf(x))) {
if (x == std.math.minValue(@typeOf(x))) {

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

We are already in the math namespace here, so no need for std.math

assert(@maxValue(u16) == 65535);
assert(@maxValue(u32) == 4294967295);
assert(@maxValue(u64) == 18446744073709551615);
test "std.math.minValue and std.math.maxValue" {

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

This should probably exist in std.math. I'm pretty sure these tests test language features.

This comment has been minimized.

Copy link
@kristate

kristate Sep 5, 2018

Author Contributor

I thought so too, but I am glad that everyone agrees -- I will move it over to std.math

@@ -1,6 +1,24 @@
const tests = @import("tests.zig");

pub fn addCases(cases: *tests.CompileErrorContext) void {
cases.add(

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

Pretty sure this tests compiler errors for compiler features, so these should not be here anymore.

This comment has been minimized.

Copy link
@kristate

kristate Sep 5, 2018

Author Contributor

std.math.minValue and friends emits a @compileError for using types that are not an Integer -- this was supposed to test that, but where would be the better place?

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

I don't think std tests for compiler errors for any of its functions. We probably just have to wait for some feature that will allow for this (I feel like I've seen an issue for it somewhere, but have only found #1356 and #567)

@andrewrk andrewrk added this to the 0.4.0 milestone Sep 5, 2018

};
},
};
}
}

test "std.math.minInt and std.math.maxInt" {
assert(std.math.maxInt(u1) == 1);

This comment has been minimized.

Copy link
@Hejsil

Hejsil Sep 5, 2018

Member

You missed removing std.math here when copy-pasting :)

@kristate kristate force-pushed the kristate:minmax-tostdlib-issue1466 branch 2 times, most recently from b5e4a9d to 01f5133 Sep 5, 2018

@kristate

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

Removed cached values and rebased.

@tiehuis

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

Could you rebase all the renames across files into one commit? It helps when going through commit logs where I think its better to see that the commit intent is highlighted rather than the individual files, which we have the diff for anyway. Thanks.

@kristate kristate force-pushed the kristate:minmax-tostdlib-issue1466 branch from 01f5133 to ee28327 Sep 6, 2018

@kristate

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@tiehuis collapsed into 640105a

@@ -246,30 +246,10 @@ test "math.max" {
/// The result is a compile time constant.
pub fn minInt(comptime T: type) @typeOf(42) {

This comment has been minimized.

Copy link
@tgschultz

tgschultz Sep 7, 2018

Contributor

@typeOf(42) can just be comptime_int now.

This comment has been minimized.

Copy link
@kristate

kristate Sep 7, 2018

Author Contributor

Thanks!

@kristate kristate force-pushed the kristate:minmax-tostdlib-issue1466 branch 4 times, most recently from be25229 to b40ebc4 Sep 7, 2018

@kristate

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2018

Going to rebase this after 0.3.0 lands.

@andrewrk
Copy link
Member

left a comment

Squash commits please

kristate added some commits Sep 5, 2018

kristate added some commits Sep 5, 2018

@kristate kristate force-pushed the kristate:minmax-tostdlib-issue1466 branch from b40ebc4 to 5302839 Oct 10, 2018

@andrewrk andrewrk closed this in 2b395d4 Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.