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.

Sign up for GitHub

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

Jump to bottom

Optimise floating point is_finite (2x) and is_infinite (1.6x). #57353

Merged
merged 1 commit into from
Jan 13, 2019
Merged

Optimise floating point is_finite (2x) and is_infinite (1.6x). #57353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/libcore/num/f32.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,14 @@ impl f32 {
self != self
}

// FIXME(#50145): `abs` is publicly unavailable in libcore due to
// concerns about portability, so this implementation is for
// private use internally.
huonw marked this conversation as resolved.
Show resolved Hide resolved
#[inline]
fn abs_private(self) -> f32 {
f32::from_bits(self.to_bits() & 0x7fff_ffff)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary, rather than using the existing abs method (which uses the LLVM fabsf32 intrinsic directly)?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's only available in std. #50145

Copy link
Member

@varkor varkor Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm bikeshedding here, but given typical naming conventions, I think it'd make sense to simply call this abs (which shouldn't cause any conflicts) and add a comment explaining why this method is private for discoverability, e.g.

// FIXME(#50145): `abs` is publicly unavailable in libcore due to concerns
// about portability, so this implementation is for private use internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree about adding a comment, but I don't think that naming it abs is appropriate. I'd call it something like abs_hack so it's clear that it's not using the proper abs method.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about abs_private with a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added that.

}

/// Returns `true` if this value is positive infinity or negative infinity and
/// false otherwise.
///
Expand All @@ -181,7 +189,7 @@ impl f32 {
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn is_infinite(self) -> bool {
self == INFINITY || self == NEG_INFINITY
self.abs_private() == INFINITY
}

/// Returns `true` if this number is neither infinite nor `NaN`.
Expand All @@ -203,7 +211,9 @@ impl f32 {
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn is_finite(self) -> bool {
!(self.is_nan() || self.is_infinite())
// There's no need to handle NaN separately: if self is NaN,
// the comparison is not true, exactly as desired.
self.abs_private() < INFINITY
}

/// Returns `true` if the number is neither zero, infinite,
Expand Down
14 changes: 12 additions & 2 deletions src/libcore/num/f64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,14 @@ impl f64 {
self != self
}

// FIXME(#50145): `abs` is publicly unavailable in libcore due to
// concerns about portability, so this implementation is for
// private use internally.
#[inline]
fn abs_private(self) -> f64 {
f64::from_bits(self.to_bits() & 0x7fff_ffff_ffff_ffff)
}

/// Returns `true` if this value is positive infinity or negative infinity and
/// false otherwise.
///
Expand All @@ -181,7 +189,7 @@ impl f64 {
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn is_infinite(self) -> bool {
self == INFINITY || self == NEG_INFINITY
self.abs_private() == INFINITY
}

/// Returns `true` if this number is neither infinite nor `NaN`.
Expand All @@ -203,7 +211,9 @@ impl f64 {
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn is_finite(self) -> bool {
!(self.is_nan() || self.is_infinite())
// There's no need to handle NaN separately: if self is NaN,
// the comparison is not true, exactly as desired.
self.abs_private() < INFINITY
}

/// Returns `true` if the number is neither zero, infinite,
Expand Down