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

implement LowerExp and UpperExp for integers #66721

Merged
merged 1 commit into from Feb 15, 2020
Merged

Conversation

@maxbla
Copy link
Contributor

maxbla commented Nov 24, 2019

Addresses #39479

This implementation is heavily based on the preexisting macro_rules! impl_Display in the same file. I don't like the liberal use of unsafe in that macro and would like to modify it so unsafe is only present where necessary. What is Rust's policy on doing such modifications?

Also, I couldn't figure out where to put tests, can I have some help with that?

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 24, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (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. Due to 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 the contribution instructions for more information.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 24, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-24T22:44:42.5388500Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-24T22:44:42.5577716Z ##[command]git config gc.auto 0
2019-11-24T22:44:42.5657486Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-24T22:44:42.5703578Z ##[command]git config --get-all http.proxy
2019-11-24T22:44:42.5854532Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66721/merge:refs/remotes/pull/66721/merge
---
2019-11-24T22:49:55.8088732Z   = note: `#[warn(unused_imports)]` on by default
2019-11-24T22:49:55.8092986Z 
2019-11-24T22:50:05.6018403Z     Finished release [optimized] target(s) in 1m 18s
2019-11-24T22:50:05.6100531Z tidy check
2019-11-24T22:50:06.4316935Z tidy error: /checkout/src/libcore/fmt/num.rs:264: line longer than 100 chars
2019-11-24T22:50:06.4317701Z tidy error: /checkout/src/libcore/fmt/num.rs:314: line longer than 100 chars
2019-11-24T22:50:07.8744686Z some tidy checks failed
2019-11-24T22:50:07.8744792Z Found 485 error codes
2019-11-24T22:50:07.8745138Z Found 0 error codes with no tests
2019-11-24T22:50:07.8745187Z Done!
2019-11-24T22:50:07.8745187Z Done!
2019-11-24T22:50:07.8748692Z 
2019-11-24T22:50:07.8749013Z 
2019-11-24T22:50:07.8749955Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-24T22:50:07.8750466Z 
2019-11-24T22:50:07.8750619Z 
2019-11-24T22:50:07.8752861Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-24T22:50:07.8753115Z Build completed unsuccessfully in 0:01:21
2019-11-24T22:50:07.8753115Z Build completed unsuccessfully in 0:01:21
2019-11-24T22:50:07.8790615Z == clock drift check ==
2019-11-24T22:50:07.8797574Z   local time: Sun Nov 24 22:50:07 UTC 2019
2019-11-24T22:50:08.1685606Z   network time: Sun, 24 Nov 2019 22:50:08 GMT
2019-11-24T22:50:08.1689515Z == end clock drift check ==
2019-11-24T22:50:09.5912095Z 
2019-11-24T22:50:09.6003494Z ##[error]Bash exited with code '1'.
2019-11-24T22:50:09.6025858Z ##[section]Starting: Checkout
2019-11-24T22:50:09.6027324Z ==============================================================================
2019-11-24T22:50:09.6027388Z Task         : Get sources
2019-11-24T22:50:09.6027427Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Alexendoo

This comment has been minimized.

Copy link
Member

Alexendoo commented Dec 4, 2019

Ping from triage, any updates? @kennytm

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 4, 2019

@Alexendoo This is a Draft PR, it is waiting for author to click "Ready for review".

@kennytm kennytm added the T-libs label Dec 4, 2019
@maxbla maxbla marked this pull request as ready for review Dec 5, 2019
@maxbla

This comment has been minimized.

Copy link
Contributor Author

maxbla commented Dec 5, 2019

Sorry, this is my first PR. I was waiting on an answer to the questions higher in the thread, as I knew it would not pass review without some changes. I have now marked it ready for review, because review will probably involve answers to the question of where to put tests.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Dec 7, 2019

@kennytm if you can answer the above question(s) we can proceed with this PR :) thanks

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 7, 2019

@maxbla Unit tests for libcore should be placed in src/libcore/tests/. For this PR, try src/libcore/tests/fmt/num.rs.

src/libcore/fmt/num.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 10, 2019

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-10T20:32:37.0351781Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-10T20:32:37.0546542Z ##[command]git config gc.auto 0
2019-12-10T20:32:37.7335452Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-10T20:32:37.7344626Z ##[command]git config --get-all http.proxy
2019-12-10T20:32:37.7350091Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66721/merge:refs/remotes/pull/66721/merge
---
2019-12-10T20:38:52.8853900Z Found 0 error codes with no tests
2019-12-10T20:38:52.8854353Z Done!
2019-12-10T20:38:52.8854500Z 
2019-12-10T20:38:52.8854827Z 
2019-12-10T20:38:52.8856405Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-12-10T20:38:52.8856841Z 
2019-12-10T20:38:52.8856964Z 
2019-12-10T20:38:52.8857116Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-12-10T20:38:52.8857292Z Build completed unsuccessfully in 0:01:32
2019-12-10T20:38:52.8857292Z Build completed unsuccessfully in 0:01:32
2019-12-10T20:38:52.8857436Z some tidy checks failed
2019-12-10T20:38:52.8890142Z == clock drift check ==
2019-12-10T20:38:52.8912970Z   local time: Tue Dec 10 20:38:52 UTC 2019
2019-12-10T20:38:53.1860830Z   network time: Tue, 10 Dec 2019 20:38:53 GMT
2019-12-10T20:38:53.1866832Z == end clock drift check ==
2019-12-10T20:38:54.5206970Z 
2019-12-10T20:38:54.5314071Z ##[error]Bash exited with code '1'.
2019-12-10T20:38:54.5342081Z ##[section]Starting: Checkout
2019-12-10T20:38:54.5343602Z ==============================================================================
2019-12-10T20:38:54.5343650Z Task         : Get sources
2019-12-10T20:38:54.5343693Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 25, 2019

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-25T08:21:03.3493590Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-25T08:21:03.3717879Z ##[command]git config gc.auto 0
2019-12-25T08:21:03.3808891Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-25T08:21:03.3873511Z ##[command]git config --get-all http.proxy
2019-12-25T08:21:03.4038220Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66721/merge:refs/remotes/pull/66721/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@maxbla

This comment has been minimized.

Copy link
Contributor Author

maxbla commented Dec 26, 2019

81305e2 adds precision support using flt2dec::Part and fmt::pad_formatted_parts().

It feels a tad weird to use a module called flt2dec for integer formatting. I think core::num::flt2dec::Part might fit better in core::fmt and core::num::flt2dec::Formatted definitely would fit better there.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 26, 2019

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm Dec 26, 2019
@maxbla

This comment has been minimized.

Copy link
Contributor Author

maxbla commented Dec 27, 2019

core::fmt::{pad_formatted_parts. write_formatted_parts} both call unsafe fn str::from_utf8_unchecked() on their arguments, but are not marked as unsafe themselves. Is it possible this was done for a reason, or can I change that?

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Jan 15, 2020

@rust-highfive rust-highfive assigned KodrAus and unassigned dtolnay Jan 15, 2020
@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Jan 15, 2020

It feels a tad weird to use a module called flt2dec for integer formatting. I think core::num::flt2dec::Part might fit better in core::fmt and core::num::flt2dec::Formatted definitely would fit better there.

I think the original implementation came from an external library so it all ended up living together. I wouldn't be against refactoring out commonly useful pieces from it.

Copy link
Contributor

KodrAus left a comment

Thanks for the PR @maxbla!

I've just left a few comments to start.

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

macro_rules! impl_Exp {
($($t:ident),* as $u:ident via $conv_fn:ident named $name:ident) => {
fn $name(

This comment has been minimized.

Copy link
@KodrAus

KodrAus Jan 15, 2020

Contributor

Before digging too deeply into the code itself can you offer any background on this implementation? Whether it's based on any common approaches used elsewhere?

This comment has been minimized.

Copy link
@maxbla

maxbla Jan 16, 2020

Author Contributor

It is heavily based on impl_Display! from the same file. It also makes use of some stuff from float's implementation (for precision).

src/libcore/fmt/num.rs Outdated Show resolved Hide resolved
src/libcore/fmt/num.rs Outdated Show resolved Hide resolved
@KodrAus
KodrAus approved these changes Feb 9, 2020
Copy link
Contributor

KodrAus left a comment

Alrighty, this is looking good to me! Thanks for working on this @maxbla!

Would you like to squash your commits down and I'll merge this one in?

@maxbla

This comment has been minimized.

Copy link
Contributor Author

maxbla commented Feb 10, 2020

@KodrAus Thanks for the review. I'm excited to get my first contribution merged!
Is now a good time to move core::num::flt2dec::Part and core::num::flt2dec::Formatted to core::fmt or did I miss the window on that?

@maxbla

This comment has been minimized.

Copy link
Contributor Author

maxbla commented Feb 15, 2020

I think this PR is now ready to merge.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Feb 15, 2020

Not yet xD

We don't do merges here so it will be nice if you could revert the merge and rebase instead.

@maxbla

This comment has been minimized.

Copy link
Contributor Author

maxbla commented Feb 15, 2020

Alright I think I did it right this time

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Feb 15, 2020

Thanks for your patience @maxbla! This looks good to me.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2020

📌 Commit a8fe47d has been approved by KodrAus

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 15, 2020
implement LowerExp and UpperExp for integers

Addresses rust-lang#39479

This implementation is heavily based on the preexisting `macro_rules! impl_Display` in the same file. I don't like the liberal use of unsafe in that macro and would like to modify it so `unsafe` is only present where necessary. What is Rust's policy on doing such modifications?

Also, I couldn't figure out where to put tests, can I have some help with that?
bors added a commit that referenced this pull request Feb 15, 2020
Rollup of 6 pull requests

Successful merges:

 - #64069 (Added From<Vec<NonZeroU8>> for CString)
 - #66721 (implement LowerExp and UpperExp for integers)
 - #69106 (Fix std::fs::copy on WASI target)
 - #69154 (Avoid calling `fn_sig` on closures)
 - #69166 (Check `has_typeck_tables` before calling `typeck_tables_of`)
 - #69180 (Suggest a comma if a struct initializer field fails to parse)

Failed merges:

r? @ghost
@bors bors merged commit a8fe47d into rust-lang:master Feb 15, 2020
4 checks passed
4 checks passed
pr Build #20200215.5 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@cuviper cuviper mentioned this pull request Feb 25, 2020
3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.