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

NextUp & NextDown #3049

Merged
merged 7 commits into from
Jul 6, 2021
Merged

NextUp & NextDown #3049

merged 7 commits into from
Jul 6, 2021

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Jul 5, 2021

We don’t have std::bit_cast in our *nix builds yet, and absl::bit_cast is not constexpr so it does not help.
Implement an IEEE 754 nextUp and nextDown 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 with std::bit_cast though.

Copy link
Member

@pleroy pleroy left a 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"
Copy link
Member

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?

Copy link
Member Author

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,
Copy link
Member

Choose a reason for hiding this comment

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

Weird camelCase (2x).

Sorry, something went wrong.

Copy link
Member Author

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.

Sorry, something went wrong.

return std::numeric_limits<SourceFormat>::denorm_min();
}
if (x == std::numeric_limits<SourceFormat>::infinity()) {
// TODO(egg): Should we be signalling the overflow exception?
Copy link
Member

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...

Copy link
Member Author

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);
// [...]
Copy link
Member

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.

Copy link
Member Author

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 +∞.

@eggrobin eggrobin mentioned this pull request Jul 6, 2021
@pleroy pleroy added the LGTM label Jul 6, 2021
@eggrobin eggrobin merged commit 346ad31 into mockingbirdnest:master Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants