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

use `sign` variable in abs and wrapping_abs methods #64386

Open
wants to merge 1 commit into
base: master
from

Conversation

@tspiteri
Copy link
Contributor

commented Sep 11, 2019

This also makes the code easier to understand by hinting at the significance of self >> ($BITS - 1).

Also, now overflowing_abs simply uses wrapping_abs, which is clearer and avoids a potential performance regression in the LLVM IR.

This PR follows from the discussion from #63786.

r? @eddyb
cc @nikic

@@ -1402,7 +1402,8 @@ $EndFeature, "
#[stable(feature = "no_panic_abs", since = "1.13.0")]
#[inline]
pub const fn wrapping_abs(self) -> Self {
(self ^ (self >> ($BITS - 1))).wrapping_sub(self >> ($BITS - 1))
let sign = self >> ($BITS - 1);
(self ^ sign).wrapping_sub(sign)

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 12, 2019

Member

Can you write a comment showing what this expression does for each of the possible two cases? If I understand correctly:

  • sign == -1: (!self).wrapping_sub(-1) aka (wrapping) !self + 1 aka -self (this is true for 2's complement but not obvious!)
  • sign == 0: self
@@ -1969,7 +1970,8 @@ $EndFeature, "
// Note that the #[inline] above means that the overflow
// semantics of the subtraction depend on the crate we're being
// inlined into.
(self ^ (self >> ($BITS - 1))) - (self >> ($BITS - 1))
let sign = self >> ($BITS - 1);
(self ^ sign) - sign

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 12, 2019

Member

Same here, but this is trickier because we want this to panic whenever -self would've, which is self == Self::min_value().

Self::min_value() only has the sign bit set, so after flipping all the bits (^ -1 aka !) you get Self::max_value() (which has all bits set except the sign bit).
And then + 1 (- (-1) technically) would make it panic, so the behavior is preserved (but this is even less obvious!).

I guess we don't print the actual values? Because if we did, they would look wrong (overflow on subtraction instead of on negation).

This comment has been minimized.

Copy link
@tspiteri

tspiteri Sep 12, 2019

Author Contributor

I had written something in the comment for the original PR. It would have been much better if I had included that in the code instead; I'm on it.

This comment has been minimized.

Copy link
@tspiteri

tspiteri Sep 12, 2019

Author Contributor

Done.

@eddyb

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@tspiteri tspiteri force-pushed the tspiteri:const-abs2 branch from cc8222e to fa7c6c4 Sep 12, 2019

@@ -1402,7 +1402,16 @@ $EndFeature, "
#[stable(feature = "no_panic_abs", since = "1.13.0")]
#[inline]
pub const fn wrapping_abs(self) -> Self {
(self ^ (self >> ($BITS - 1))).wrapping_sub(self >> ($BITS - 1))
// sign is -1 (all ones) for -ve numbers, 0 otherwise.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Sep 12, 2019

Member

Thanks for adding comments! However, please spell out "negative" and "positive", these abbreviations are fairly hard to decipher.

(That said, this does not really alleviate my concern about sacrificing code readability to please const qualification.)

This comment has been minimized.

Copy link
@tspiteri

tspiteri Sep 13, 2019

Author Contributor

I removed the abbreviations.

About readability, I do not share your concern. While, all other things being equal, simpler code is better, I don't think it is enough reason to delay better functionality, specifically I don't think it is enough reason to delay making the abs functions const until the time when const support in the compiler improves. I also think having this functionality in the standard library is helpful as it avoids having such tricks repeated in user code, where it is more probable that typos get in somewhere.

And in this case the normal issues with less obvious code are not so problematic in my opinion: Modifiability is not such an issue, because there is very little scope for modifying the abs functions. And the risk of getting incorrect behaviour is extremely tiny (I want to say there is no risk in this case, but there's always the remote chance that this change triggers some hidden bug in some other part of the compiler, so I'll stay with extremely tiny ☺).

The comments help as they make it possible (though not necessarily trivial) for even someone with a very limited knowledge of two's-complement representation and bitwise manipulation to verify the correctness of the code.

use `sign` variable in abs and wrapping_abs methods
This also makes the code easier to understand by hinting at the
significance of `self >> ($BITS - 1)` and by including an explanation
in the comments.

Also, now overflowing_abs simply uses wrapping_abs, which is clearer
and avoids a potential performance regression in the LLVM IR.

@tspiteri tspiteri force-pushed the tspiteri:const-abs2 branch from fa7c6c4 to 562903a Sep 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.