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

Much ado about nothing’s sign bit #2372

Merged

Conversation

eggrobin
Copy link
Member

No description provided.

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()) {
Copy link
Member

Choose a reason for hiding this comment

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

is_negative?

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, that's an optional sign on the left hand side.

geometry/sign.hpp Show resolved Hide resolved
geometry/sign.hpp Show resolved Hide resolved
geometry/sign.hpp Outdated Show resolved Hide resolved
// Returns ±1.
constexpr operator int() const;

constexpr bool operator==(Sign const& other) const;
Copy link
Member

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?

Copy link
Member Author

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.

Sorry, something went wrong.

};

Sign operator*(Sign const& left, Sign const& right);
constexpr Sign operator*(Sign const& left, Sign const& right);
Copy link
Member

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.

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.

A job for another pull request, we should harmonize Quantity with Multivector as well.

geometry/sign_body.hpp Outdated Show resolved Hide resolved
geometry/sign_body.hpp Outdated Show resolved Hide resolved
constexpr Sign::Sign(bool negative) : negative_(negative) {}

constexpr Sign operator*(Sign const& left, Sign const& right) {
return Sign(/*negative=*/left.negative_ != right.negative_);
Copy link
Member

Choose a reason for hiding this comment

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

Use ^ instead of !=.

Copy link
Member Author

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.

@pleroy pleroy added the LGTM label Nov 16, 2019
@eggrobin eggrobin merged commit 8fabcf6 into mockingbirdnest:master Nov 16, 2019
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