-
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
Much ado about nothing’s sign bit #2372
The head ref may contain hidden characters: "much-ado-about-nothing\u2019s-sign-bit"
Much ado about nothing’s sign bit #2372
Conversation
xy_displacement_derivative_sign == Sign(1) && | ||
last_xy_displacement_derivative_sign == Sign(-1)) { | ||
xy_displacement_derivative_sign.is_positive() && | ||
last_xy_displacement_derivative_sign == Sign::Negative()) { |
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.
is_negative
?
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, that's an optional sign on the left hand side.
geometry/sign.hpp
Outdated
// Returns ±1. | ||
constexpr operator int() const; | ||
|
||
constexpr bool operator==(Sign const& other) const; |
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.
Sign
should probably be passed by copy. I mean, it's one bool, what could it cost? Ten bytes?
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.
In general we should definitely revisit our const&
usage; we pass quantities by const&
, and that is probably silly as well.
Done.
geometry/sign.hpp
Outdated
}; | ||
|
||
Sign operator*(Sign const& left, Sign const& right); | ||
constexpr Sign operator*(Sign const& left, Sign const& right); |
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 do we have *
here but ==
and !=
in the class? We should probably decide once and for all what we do with binary operators.
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 job for another pull request, we should harmonize Quantity
with Multivector
as well.
inline bool Sign::Negative() const { | ||
return negative_; | ||
// TODO(egg): Consider intrinsics. | ||
inline Sign::Sign(double x) : negative_(std::signbit(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.
const
constexpr Sign::Sign(bool negative) : negative_(negative) {} | ||
|
||
constexpr Sign operator*(Sign const& left, Sign const& right) { | ||
return Sign(/*negative=*/left.negative_ != right.negative_); |
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.
Use ^
instead of !=
.
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.
^
is an arithmetic operator, not a logical one, so this requires a cast; from the point of view of readability, do we want that?
In terms of generated code, it does appear to make a difference with MSVC, but only if ^
is applied to signed operands: https://gcc.godbolt.org/z/dGvY55; it makes no difference with clang.
No description provided.