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

make is_power_of_two a const function #65092

Merged
merged 2 commits into from Oct 22, 2019

Conversation

@tspiteri
Copy link
Contributor

commented Oct 4, 2019

This makes is_power_of_two a const function by using & instead of short-circuiting &&; Rust supports bitwise & for bool and short-circuiting is not required in the existing expression.

I don't think this needs a const-hack label as I don't find the changed code less readable, if anything I prefer that it is clearer that short circuiting is not used.

@oli-obk

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2019

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

@tspiteri

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@rust-highfive rust-highfive assigned oli-obk and unassigned Kimundi Oct 4, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

This change may have performance implications, though maybe even be an improvement since it's now branchless code (though likely llvm figured this out by itself anyway).

@rust-lang/libs are you fine with insta stabilizing the constness here?

@pitdicker

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

The result in compiler explorer with & and && is the same: https://godbolt.org/z/S30HDY

@tspiteri

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

Yeah, LLVM generates the same IR for both.

@alexcrichton alexcrichton added the T-libs label Oct 4, 2019
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

@rfcbot fcp merge

To confirm the insta-stable const-ness...

@rfcbot

This comment has been minimized.

Copy link

commented Oct 4, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Centril

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@rfcbot

This comment has been minimized.

Copy link

commented Oct 11, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@RalfJung

This comment has been minimized.

Copy link
Member

commented Oct 17, 2019

I don't think this needs a const-hack label as I don't find the changed code less readable, if anything I prefer that it is clearer that short circuiting is not used.

The purpose of the const-hack label is to mark changes that we'll want to revert later, once the original code works in const fn.

So, will we want to revert this? My personal thinking is yes; & on bool is unconventional and (except for const-qualif) there is no reason to use it here. So I am marking this as const-hack; once we do the revert we can still discuss if we wouldn't rather keep the code as it is then.

@RalfJung RalfJung added the const-hack label Oct 17, 2019
@tspiteri

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2019

So, will we want to revert this? My personal thinking is yes; & on bool is unconventional

If it is to be eventually changed to avoid & on bool (I'm neutral on that), the original order is strange. If anything the comparison of self to 0 would be done on the LHS to short circuit the possibility of wrapping, and then wrapping_sub could be replaced with a subtraction, as in self != 0 && (self - 1) & self == 0.

@tspiteri

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2019

Or maybe self.count_ones() == 1. (Incidentally, with optimizations LLVM recognizes this pattern in all of the above implementations, so that the IR is a call to @llvm.ctpop followed by a comparison to 1.)

@tspiteri

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2019

Once FCP for the insta-stable const-ness is over, I could update the method to simply return self.count_ones() == 1 if required.

(LLVM produces identical IR so there should be no performance issue, and I think it is more readable than the current and the modified versions, which are basically doing the same thing using some bitwise hackery. And count_ones is already const, so this would result in a const function without the need for const-hack.)

@nikic nikic referenced this pull request Oct 20, 2019
src/libcore/num/mod.rs Outdated Show resolved Hide resolved
@rfcbot

This comment has been minimized.

Copy link

commented Oct 21, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@tspiteri

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2019

@oli-obk Now that the RFC for the insta-stable constness is over, should I change the body of the function to self.count_ones() == 1 (which I find most readable and would, I think, make const-hack unnecessary), or accept the style nits fix suggested by @lzutao?

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2019

I like the count_ones solution the best.

@tspiteri tspiteri force-pushed the tspiteri:const-is-pow2 branch from a12788a to 354003e Oct 21, 2019
@tspiteri tspiteri force-pushed the tspiteri:const-is-pow2 branch from 354003e to d689c70 Oct 21, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2019

📌 Commit d689c70 has been approved by oli-obk

@RalfJung RalfJung removed the const-hack label Oct 21, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 21, 2019
make is_power_of_two a const function

This makes `is_power_of_two` a const function by using `&` instead of short-circuiting `&&`; Rust supports bitwise `&` for `bool` and short-circuiting is not required in the existing expression.

I don't think this needs a const-hack label as I don't find the changed code less readable, if anything I prefer that it is clearer that short circuiting is not used.

@oli-obk
bors added a commit that referenced this pull request Oct 21, 2019
Rollup of 7 pull requests

Successful merges:

 - #62330 (Change untagged_unions to not allow union fields with drop)
 - #65092 (make is_power_of_two a const function)
 - #65501 (Remove `src/llvm-emscripten` submodule)
 - #65621 (miri: add write_bytes method to Memory doing bounds-checks and supporting iterators)
 - #65647 (Remove unnecessary trait bounds and derivations)
 - #65653 (keep the root dir clean from debugging)
 - #65663 (Fix typo from #65214)

Failed merges:

 - #65660 (Rename `ConstValue::Infer(InferConst::Canonical(..))` to `ConstValue::Bound(..)`)

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Oct 21, 2019
make is_power_of_two a const function

This makes `is_power_of_two` a const function by using `&` instead of short-circuiting `&&`; Rust supports bitwise `&` for `bool` and short-circuiting is not required in the existing expression.

I don't think this needs a const-hack label as I don't find the changed code less readable, if anything I prefer that it is clearer that short circuiting is not used.

@oli-obk
bors added a commit that referenced this pull request Oct 21, 2019
Rollup of 7 pull requests

Successful merges:

 - #62330 (Change untagged_unions to not allow union fields with drop)
 - #65092 (make is_power_of_two a const function)
 - #65621 (miri: add write_bytes method to Memory doing bounds-checks and supporting iterators)
 - #65647 (Remove unnecessary trait bounds and derivations)
 - #65653 (keep the root dir clean from debugging)
 - #65660 (Rename `ConstValue::Infer(InferConst::Canonical(..))` to `ConstValue::Bound(..)`)
 - #65663 (Fix typo from #65214)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Oct 22, 2019
Rollup of 7 pull requests

Successful merges:

 - #62330 (Change untagged_unions to not allow union fields with drop)
 - #65092 (make is_power_of_two a const function)
 - #65621 (miri: add write_bytes method to Memory doing bounds-checks and supporting iterators)
 - #65647 (Remove unnecessary trait bounds and derivations)
 - #65653 (keep the root dir clean from debugging)
 - #65660 (Rename `ConstValue::Infer(InferConst::Canonical(..))` to `ConstValue::Bound(..)`)
 - #65663 (Fix typo from #65214)

Failed merges:

r? @ghost
@bors bors merged commit d689c70 into rust-lang:master Oct 22, 2019
4 checks passed
4 checks passed
pr Build #20191021.37 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@tspiteri tspiteri deleted the tspiteri:const-is-pow2 branch Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.