make is_power_of_two a const function by tspiteri · Pull Request #65092 · rust-lang/rust

tspiteri · 2019-10-04T09:15:50Z

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 · 2019-10-04T09:15:53Z

r? @Kimundi

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

tspiteri · 2019-10-04T09:16:59Z

r? @oli-obk

oli-obk · 2019-10-04T09:56:04Z

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 · 2019-10-04T09:59:51Z

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

tspiteri · 2019-10-04T10:30:50Z

Yeah, LLVM generates the same IR for both.

alexcrichton · 2019-10-04T14:43:19Z

@rfcbot fcp merge

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

rfcbot · 2019-10-04T14:44:02Z

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 · 2019-10-11T11:29:05Z

👋 @Amanieu @Kimundi @withoutboats

rfcbot · 2019-10-11T12:58:49Z

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

RalfJung · 2019-10-17T14:50:20Z

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.

tspiteri · 2019-10-17T15:20:21Z

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 · 2019-10-17T15:27:11Z

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 · 2019-10-18T13:11:33Z

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

src/libcore/num/mod.rs

rfcbot · 2019-10-21T13:00:48Z

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 · 2019-10-21T13:09:37Z

@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 · 2019-10-21T13:24:32Z

I like the count_ones solution the best.

oli-obk · 2019-10-21T13:37:37Z

@bors r+

bors · 2019-10-21T13:37:38Z

📌 Commit d689c70 has been approved by oli-obk

@oli-obk

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

@oli-obk

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

@ghost

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

Version 1.40.0 (2019-12-19) =========================== Language -------- - [You can now use tuple `struct`s and tuple `enum` variant's constructors in `const` contexts.][65188] e.g. ```rust pub struct Point(i32, i32); const ORIGIN: Point = { let constructor = Point; constructor(0, 0) }; ``` - [You can now mark `struct`s, `enum`s, and `enum` variants with the `#[non_exhaustive]` attribute to indicate that there may be variants or fields added in the future.][64639] For example this requires adding a wild-card branch (`_ => {}`) to any match statements on a non-exhaustive `enum`. [(RFC 2008)] - [You can now use function-like procedural macros in `extern` blocks and in type positions.][63931] e.g. `type Generated = macro!();` - [Function-like and attribute procedural macros can now emit `macro_rules!` items, so you can now have your macros generate macros.][64035] - [The `meta` pattern matcher in `macro_rules!` now correctly matches the modern attribute syntax.][63674] For example `(#[$m:meta])` now matches `#[attr]`, `#[attr{tokens}]`, `#[attr[tokens]]`, and `#[attr(tokens)]`. Compiler -------- - [Added tier 3 support\* for the `thumbv7neon-unknown-linux-musleabihf` target.][66103] - [Added tier 3 support for the `aarch64-unknown-none-softfloat` target.][64589] - [Added tier 3 support for the `mips64-unknown-linux-muslabi64`, and `mips64el-unknown-linux-muslabi64` targets.][65843] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [The `is_power_of_two` method on unsigned numeric types is now a `const` function.][65092] Stabilized APIs --------------- - [`BTreeMap::get_key_value`] - [`HashMap::get_key_value`] - [`Option::as_deref_mut`] - [`Option::as_deref`] - [`Option::flatten`] - [`UdpSocket::peer_addr`] - [`f32::to_be_bytes`] - [`f32::to_le_bytes`] - [`f32::to_ne_bytes`] - [`f64::to_be_bytes`] - [`f64::to_le_bytes`] - [`f64::to_ne_bytes`] - [`f32::from_be_bytes`] - [`f32::from_le_bytes`] - [`f32::from_ne_bytes`] - [`f64::from_be_bytes`] - [`f64::from_le_bytes`] - [`f64::from_ne_bytes`] - [`mem::take`] - [`slice::repeat`] - [`todo!`] Cargo ----- - [Cargo will now always display warnings, rather than only on fresh builds.][cargo/7450] - [Feature flags (except `--all-features`) passed to a virtual workspace will now produce an error.][cargo/7507] Previously these flags were ignored. - [You can now publish `dev-dependencies` without including a `version`.][cargo/7333] Misc ---- - [You can now specify the `#[cfg(doctest)]` attribute to include an item only when running documentation tests with `rustdoc`.][63803] Compatibility Notes ------------------- - [As previously announced, any previous NLL warnings in the 2015 edition are now hard errors.][64221] - [The `include!` macro will now warn if it failed to include the entire file.][64284] The `include!` macro unintentionally only includes the first _expression_ in a file, and this can be unintuitive. This will become either a hard error in a future release, or the behavior may be fixed to include all expressions as expected. - [Using `#[inline]` on function prototypes and consts now emits a warning under `unused_attribute` lint.][65294] Using `#[inline]` anywhere else inside traits or `extern` blocks now correctly emits a hard error. [65294]: rust-lang/rust#65294 [66103]: rust-lang/rust#66103 [65843]: rust-lang/rust#65843 [65188]: rust-lang/rust#65188 [65092]: rust-lang/rust#65092 [64589]: rust-lang/rust#64589 [64639]: rust-lang/rust#64639 [64221]: rust-lang/rust#64221 [64284]: rust-lang/rust#64284 [63931]: rust-lang/rust#63931 [64035]: rust-lang/rust#64035 [63674]: rust-lang/rust#63674 [63803]: rust-lang/rust#63803 [cargo/7450]: rust-lang/cargo#7450 [cargo/7507]: rust-lang/cargo#7507 [cargo/7525]: rust-lang/cargo#7525 [cargo/7333]: rust-lang/cargo#7333 [(rfc 2008)]: https://rust-lang.github.io/rfcs/2008-non-exhaustive.html [`f32::to_be_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.to_be_bytes [`f32::to_le_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.to_le_bytes [`f32::to_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.to_ne_bytes [`f64::to_be_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.to_be_bytes [`f64::to_le_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.to_le_bytes [`f64::to_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.to_ne_bytes [`f32::from_be_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.from_be_bytes [`f32::from_le_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.from_le_bytes [`f32::from_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.from_ne_bytes [`f64::from_be_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.from_be_bytes [`f64::from_le_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.from_le_bytes [`f64::from_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.from_ne_bytes [`option::flatten`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.flatten [`option::as_deref`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.as_deref [`option::as_deref_mut`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.as_deref_mut [`hashmap::get_key_value`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.get_key_value [`btreemap::get_key_value`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.get_key_value [`slice::repeat`]: https://doc.rust-lang.org/std/primitive.slice.html#method.repeat [`mem::take`]: https://doc.rust-lang.org/std/mem/fn.take.html [`udpsocket::peer_addr`]: https://doc.rust-lang.org/std/net/struct.UdpSocket.html#method.peer_addr [`todo!`]: https://doc.rust-lang.org/std/macro.todo.html

make is_power_of_two a const function

a12788a

rust-highfive assigned Kimundi Oct 4, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2019

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

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 4, 2019

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 4, 2019

Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 4, 2019

Centril added this to the 1.40 milestone Oct 4, 2019

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 11, 2019

RalfJung added the const-hack label Oct 17, 2019

nikic mentioned this pull request Oct 20, 2019

constify is_power_of_two #65628

Closed

tesuji reviewed Oct 20, 2019

View reviewed changes

src/libcore/num/mod.rs Outdated Show resolved Hide resolved

rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Oct 21, 2019

rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 21, 2019

tspiteri force-pushed the const-is-pow2 branch from a12788a to 354003e Compare October 21, 2019 13:33

improve readability of is_power_of_two

d689c70

tspiteri force-pushed the const-is-pow2 branch from 354003e to d689c70 Compare October 21, 2019 13:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2019

RalfJung removed the const-hack label Oct 21, 2019

Centril mentioned this pull request Oct 21, 2019

Rollup of 7 pull requests #65670

Closed

Centril mentioned this pull request Oct 21, 2019

Rollup of 7 pull requests #65671

Merged

bors merged commit d689c70 into rust-lang:master Oct 22, 2019

burrbull mentioned this pull request Oct 22, 2019

heapless on const generics: status rust-embedded/heapless#102

Closed

tspiteri deleted the const-is-pow2 branch October 22, 2019 05:44

TimDiekmann mentioned this pull request Nov 4, 2019

Tracking Issue for more const int functions #53718

Closed

59 tasks

Uh oh!

Conversation

tspiteri commented Oct 4, 2019

Uh oh!

rust-highfive commented Oct 4, 2019

Uh oh!

tspiteri commented Oct 4, 2019

Uh oh!

oli-obk commented Oct 4, 2019

Uh oh!

pitdicker commented Oct 4, 2019

Uh oh!

tspiteri commented Oct 4, 2019

Uh oh!

alexcrichton commented Oct 4, 2019

Uh oh!

rfcbot commented Oct 4, 2019 • edited by Amanieu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Centril commented Oct 11, 2019

Uh oh!

rfcbot commented Oct 11, 2019

Uh oh!

RalfJung commented Oct 17, 2019

Uh oh!

tspiteri commented Oct 17, 2019

Uh oh!

tspiteri commented Oct 17, 2019

Uh oh!

tspiteri commented Oct 18, 2019

Uh oh!

Uh oh!

rfcbot commented Oct 21, 2019

Uh oh!

tspiteri commented Oct 21, 2019

Uh oh!

oli-obk commented Oct 21, 2019

Uh oh!

oli-obk commented Oct 21, 2019

Uh oh!

bors commented Oct 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

rfcbot commented Oct 4, 2019 •

edited by Amanieu

Loading