Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upRange syntax distribution #9
Conversation
added some commits
Feb 6, 2015
rust-highfive
assigned
alexcrichton
Feb 6, 2015
This comment has been minimized.
This comment has been minimized.
rust-highfive
commented
Feb 6, 2015
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
This comment has been minimized.
This comment has been minimized.
|
r? @huonw |
rust-highfive
assigned
huonw
and unassigned
alexcrichton
Feb 6, 2015
This comment has been minimized.
This comment has been minimized.
|
Hi, @oli-obk, thanks for the PR, it looks great, but I'm not keen on merging it now. I took a similar approach in the design I proposed in rust-lang/rfcs#722, I'd prefer to not do a breaking change here when the intention is to move towards something more like that, that is, do one breaking change instead of two. Thanks for the PR, though! |
oli-obk commentedFeb 6, 2015
removed the
Rangetype and uses the range syntax instead.Yes, picking out single random numbers is slower than before. The subtraction and modulo are done "every" time. Inlining and optimizations should kill them in most cases. Also generating random numbers is some powers slower than the subtract and mod.
benchmarks with PR
benchmarks w/o PR (but added the same benchmark code)