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

fix std::f32 and std::f64 constants #13710

Merged
merged 1 commit into from
Apr 24, 2014
Merged

Conversation

aturon
Copy link
Member

@aturon aturon commented Apr 23, 2014

Some of the constant values in std::f32 were incorrectly copied from
std::f64. More broadly, both modules defined their constants redundantly
in two places, which is what led to the bug. Moreover, the specs for
some of the constants were incorrect, even when the values were correct.

Closes #13297. Closes #11537.

@aturon
Copy link
Member Author

aturon commented Apr 23, 2014

/cc @bjz @brson

Note: in this patch, I changed the implementation of Bounded for f32 and f64 to return the smallest and largest non-infinite float values for each. Previously, min_value was returning the smallest positive number.

@sfackler
Copy link
Member

The old implementation of min_value was consistent with C/C++: http://www.cplusplus.com/reference/cfloat/

@aturon
Copy link
Member Author

aturon commented Apr 23, 2014

@sfackler right -- note that I left consts::MIN_VALUE as it was. But the question is what the spec for the Bounded trait's min_value function should be. For the integer types, it returns the smallest representable number, which is negative, and what I would expect the "bounds" for the type to be. So it seemed that floats should take the same approach.

@brson
Copy link
Contributor

brson commented Apr 23, 2014

Thanks, @aturon.

@aturon
Copy link
Member Author

aturon commented Apr 23, 2014

@brson: agreed. Maybe consts::MIN_VALUE should be renamed to something else? (Personally, I found the C/C++ name for this constant quite confusing; I would prefer MIN_POS_VALUE).

/cc @sfackler @bjz

Some of the constant values in std::f32 were incorrectly copied from
std::f64.  More broadly, both modules defined their constants redundantly
in two places, which is what led to the bug.  Moreover, the specs for
some of the constants were incorrent, even when the values were correct.

Closes rust-lang#13297.  Closes rust-lang#11537.
@aturon
Copy link
Member Author

aturon commented Apr 23, 2014

r?

@@ -64,26 +64,23 @@ mod cmath {
}
}

// FIXME(#11621): These constants should be deprecated once CTFE is implemented
// in favour of calling their respective functions in `Bounded` and `Float`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this comment removed? It should probably be changed to being dependent on associated items rather than CTFE, but I think a mention is important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; I'll make that change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjz is there an issue or RFC for associated items? I couldn't find one via github's search. If not, perhaps best to leave as a reference to CTFE for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5527 maybe?

@brendanzab
Copy link
Member

Agreed about the min and max value names being misleading. The Bounded impl should also be removed. I would do this in a separate PR though.

bors added a commit that referenced this pull request Apr 24, 2014
Some of the constant values in std::f32 were incorrectly copied from
std::f64.  More broadly, both modules defined their constants redundantly
in two places, which is what led to the bug.  Moreover, the specs for
some of the constants were incorrect, even when the values were correct.

Closes #13297.  Closes #11537.
@bors bors closed this Apr 24, 2014
@bors bors merged commit 266812e into rust-lang:master Apr 24, 2014
@aturon aturon deleted the float-constants branch April 24, 2014 17:18
aturon added a commit to aturon/rust that referenced this pull request Apr 25, 2014
Follow-up on issue rust-lang#13297 and PR rust-lang#13710.  Instead of following the (confusing) C/C++ approach
of using `MIN_VALUE` for the smallest *positive* number, we introduce `MIN_POS_VALUE` (and
in the Float trait, `min_pos_value`) to represent this number.

This patch also removes a few remaining redundantly-defined constants that were missed last
time around.
bors added a commit that referenced this pull request Apr 25, 2014
Follow-up on issue #13297 and PR #13710.  Instead of following the (confusing) C/C++ approach
of using `MIN_VALUE` for the smallest *positive* number, we introduce `MIN_POS_VALUE` (and
in the Float trait, `min_pos_value`) to represent this number.

This patch also removes a few remaining redundantly-defined constants that were missed last
time around.
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.

std::f32 declares wrong mantissa length Some std::f32 constants are actually f64s
5 participants