-
Notifications
You must be signed in to change notification settings - Fork 69
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
NextUp & NextDown #3049
NextUp & NextDown #3049
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For scale_b
too, we should separate the body.
@@ -12,6 +12,7 @@ | |||
#include "geometry/named_quantities.hpp" | |||
#include "glog/logging.h" | |||
#include "numerics/double_precision.hpp" | |||
#include "quantities/elementary_functions.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this include? Shouldn't you get next.hpp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numerics/next.hpp
only has the one for double
.
namespace numerics { | ||
|
||
// A constexpr implementation of the IEEE 754:2008 logB function. | ||
// Uses sourceFormat as logBFormat, which makes it easy to cleanly handle NaN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird camelCase (2x).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IEEE 754 uses that case, and I am referring to names in there.
// functions. | ||
template<typename SourceFormat, | ||
typename = std::enable_if_t<std::is_floating_point_v<SourceFormat>>> | ||
constexpr SourceFormat NextUp(SourceFormat const x) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate the body.
numerics/next.hpp
Outdated
return std::numeric_limits<SourceFormat>::denorm_min(); | ||
} | ||
if (x == std::numeric_limits<SourceFormat>::infinity()) { | ||
// TODO(egg): Should we be signalling the overflow exception? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we ever use the floating-point exceptions...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but when it’s trivial to get them right we might as well…
static_assert(NextUp(-std::numeric_limits<double>::infinity()) == | ||
-0x1.FFFFFFFFFFFFFp+1023); | ||
static_assert(NextUp(-0x1.FFFFFFFFFFFFFp+1023) == -0x1.FFFFFFFFFFFFEp+1023); | ||
// [...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea what this comment is trying to tell me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few numbers were omitted in the progression from -∞ to +∞.
We don’t have
std::bit_cast
in our *nix builds yet, andabsl::bit_cast
is not constexpr so it does not help.Implement an IEEE 754
nextUp
andnextDown
the slow and ugly way, using arithmetic.We will probably want to keep those even when we get C++20 everywhere, since
std::nextafter
does not have IEEE 754 semantics and is not constexpr anyway. We will want to reimplement them withstd::bit_cast
though.