rustfmt the libcore/num #29113

mkpankov · 2015-10-16T23:13:53Z

rust-highfive · 2015-10-16T23:14:00Z

r? @aturon

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

mkpankov · 2015-10-16T23:15:12Z

r? @nrc

mkpankov · 2015-10-16T23:16:22Z

https://github.com/nrc/rustfmt/issues/476 and https://github.com/nrc/rustfmt/issues/477 are filed in the process

hanna-kruppe · 2015-10-17T07:22:40Z

src/libcore/num/flt2dec/mod.rs

@@ -425,26 +488,40 @@ pub fn to_shortest_str<'a, T, F>(mut format_shortest: F, v: T,
    match full_decoded {
        FullDecoded::Nan => {
            parts[0] = Part::Copy(b"NaN");
-            Formatted { sign: sign, parts: &parts[..1] }


nrc/rustfmt#418

This and many other struct literals in the diff look better on a single line IMHO.

This is a separate issue. By default, structs that have bodies longer than 20 chars are formatted on multiple lines. This can be changed in the configuration file.

Right, separate issue, but what do you think about increasing that threshold (or otherwise altering the heuristic)?

I am not opposed to the idea. You could open an issue for it.

Ok, should I revert some of such changes then?

This alone shouldn't block the PR in my opinion. When and if this rustfmt default changes, a lot of code formatted earlier will have to be re-rustfmt'd anyway.

hanna-kruppe · 2015-10-17T13:30:53Z

When I tentatively ran rustfmt over some of these files, it also changed the formatting of the auto-generated num/dec2flt/table.rs. Did that change recently or did you somehow tell rustfmt to ignore that file?

mkpankov · 2015-10-17T13:40:45Z

@rkruppe I just discarded those changes from the commit. Thought there's no sane heuristics to not touch such files.

mkpankov · 2015-10-17T13:57:24Z

tidy also says this:

/build/src/libstd/sys/common/unwind/mod.rs:84: unmatched SNAP line: // SNAP: i686-pc-windows-gnu

/build/src/libstd/sys/common/unwind/mod.rs:89: unmatched SNAP line: // SNAP: x86_64-pc-windows-msvc

I didn't touch these files, and the diff with master shows nothing. Should I be concerned with these messages?

nrc · 2015-10-18T21:51:46Z

@mkpankov don't worry about the SNAP lines, they have been around for a while, they're not due to your changes.

nrc · 2015-10-18T21:53:04Z

Better to use git rebase on your local branch rather than merging.

nrc · 2015-10-18T21:56:58Z

src/libcore/num/flt2dec/strategy/grisu.rs

@@ -430,7 +481,7 @@ pub fn format_shortest_opt(d: &Decoded,
 /// The shortest mode implementation for Grisu with Dragon fallback.
 ///
 /// This should be used for most cases.
-pub fn format_shortest(d: &Decoded, buf: &mut [u8]) -> (/*#digits*/ usize, /*exp*/ i16) {
+pub fn format_shortest(d: &Decoded, buf: &mut [u8]) -> (usize, i16) {


Lost comments here, they should be moved outside the decl, I guess (although a very recent change to rustfmt might have fixed this).

nrc · 2015-10-18T21:58:42Z

re num/dec2flt/table.rs, you should be able to add #[rustfmt_skip] to the mod table; declaration wherever that is to prevent rustfmt seeing it.

nrc · 2015-10-18T21:59:39Z

Probably worth re-formatting with the latest version of rustfmt, it should fix a few minor issues. r+ with the issues I commented on addressed.

mkpankov · 2015-10-21T23:16:38Z

@nrc Amended with results of latest rustfmt; there were much more changes than we probably expected, because of comments reformatting.

I'm still resolving issues you commented on, please hold on commenting :)

Also, as for rebase: I will take that into account in future. Now I decided to continue to use merge as rebase would screw up the previously made merge.

mkpankov · 2015-10-21T23:20:16Z

Changes I explicitly discarded are below. Maybe some of them are worth filing a bug.

diff --git a/src/libcore/num/bignum.rs b/src/libcore/num/bignum.rs
index dac7193..ca3419b 100644
--- a/src/libcore/num/bignum.rs
+++ b/src/libcore/num/bignum.rs
@@ -104,11 +104,7 @@ impl_full_ops! {

 /// Table of powers of 5 representable in digits. Specifically, the largest {u8, u16, u32} value
 /// that's a power of five, plus the corresponding exponent. Used in `mul_pow5`.
-const SMALL_POW5: [(u64, usize); 3] = [
-    (125, 3),
-    (15625, 6),
-    (1_220_703_125, 13),
-];
+const SMALL_POW5: [(u64, usize); 3] = [(125, 3), (15625, 6), (1_220_703_125, 13)];

 macro_rules! define_bignum {
     ($name:ident: type=$ty:ty, n=$n:expr) => (
diff --git a/src/libcore/num/diy_float.rs b/src/libcore/num/diy_float.rs
index 4a8332b..01fbcdc 100644
--- a/src/libcore/num/diy_float.rs
+++ b/src/libcore/num/diy_float.rs
@@ -40,7 +40,7 @@ impl Fp {
         let bc = b * c;
         let ad = a * d;
         let bd = b * d;
-        let tmp = (bd >> 32) + (ad & MASK) + (bc & MASK) + (1 << 31) /* round */;
+        let tmp = (bd >> 32) + (ad & MASK) + (bc & MASK) + (1 << 31);
         let f = ac + (ad >> 32) + (bc >> 32) + (tmp >> 32);
         let e = self.e + other.e + 64;
         Fp { f: f, e: e }
diff --git a/src/libcore/num/flt2dec/strategy/grisu.rs b/src/libcore/num/flt2dec/strategy/grisu.rs
index e026f3c..dcc3980 100644
--- a/src/libcore/num/flt2dec/strategy/grisu.rs
+++ b/src/libcore/num/flt2dec/strategy/grisu.rs
@@ -144,14 +144,14 @@ pub fn max_pow10_no_more_than(x: u32) -> (u8, u32) {
     debug_assert!(x > 0);

     const X9: u32 = 10_0000_0000;
-    const X8: u32 =  1_0000_0000;
-    const X7: u32 =    1000_0000;
-    const X6: u32 =     100_0000;
-    const X5: u32 =      10_0000;
-    const X4: u32 =       1_0000;
-    const X3: u32 =         1000;
-    const X2: u32 =          100;
-    const X1: u32 =           10;
+    const X8: u32 = 1_0000_0000;
+    const X7: u32 = 1000_0000;
+    const X6: u32 = 100_0000;
+    const X5: u32 = 10_0000;
+    const X4: u32 = 1_0000;
+    const X3: u32 = 1000;
+    const X2: u32 = 100;
+    const X1: u32 = 10;

     if x < X4 {
         if x < X2 {

mkpankov · 2015-10-21T23:24:49Z

Now all the issues should have been addressed. If I forgot something, please ping me! 😉

nrc · 2015-10-21T23:39:49Z

@mkpankov The comments look pretty bad, I'd be reluctant to land like this. We'll fixup rustfmt to do better here asap.

mkpankov · 2015-10-22T19:48:43Z

Oh...

How do I deal with this?

rustc: x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore

src/libcore/num/dec2flt/mod.rs:150:1: 150:16 error: unused attribute, #[deny(unused_attributes)] on by default

src/libcore/num/dec2flt/mod.rs:150 #[rustfmt_skip]

                                   ^~~~~~~~~~~~~~~

error: aborting due to previous error

make: *** [x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/stamp.core] Error 101

The command "make tidy && make check-notidy -j4" exited with 2.

nrc · 2015-10-22T20:18:32Z

You'll need to add

#![feature(custom_attribute)]
#![allow(unused_attributes)]

To the crate root.

mkpankov · 2015-10-22T23:45:47Z

What should I do with comments being wrapped in a strange way? I couldn't find an issue about that in rustfmt.

mkpankov · 2015-10-24T16:55:46Z

Okay, seems I should just drop whatever changes there are and re-run rustfmt afresh, since there's a lot of comment breakage. I'll rebase and force push new version.

mkpankov · 2015-10-24T17:08:16Z

Ah, moreover, seems I should wait on https://github.com/nrc/rustfmt/issues/503 as it's the reason comments break in the first place.

marcusklaas · 2015-10-24T17:16:39Z

That may not be fixed very soon. In the mean time, you could repair the offending comments by hand. Once comments obey the character limits, rustfmt generally won't touch them.

-----Original Message-----
From: "Michael Pankov" notifications@github.com
Sent: ‎24-‎10-‎2015 19:08
To: "rust-lang/rust" rust@noreply.github.com
Cc: "Marcus Klaas de Vries" mail@marcusklaas.nl
Subject: Re: [rust] rustfmt the libcore/num (#29113)

Ah, moreover, seems I should wait on nrc/rustfmt#503 as it's the reason comments break in the first place.
—
Reply to this email directly or view it on GitHub.

nrc · 2015-10-25T17:47:08Z

Comments issue: https://github.com/nrc/rustfmt/issues/535

nrc · 2015-10-25T17:48:09Z

@mkpankov you could either fixup the comments or wait until we have an option to not touch comments, which shouldn't be too long in coming (as opposed to a proper fix, which might take a while).

bors · 2015-11-01T09:00:23Z

☔ The latest upstream changes (presumably #29316) made this pull request unmergeable. Please resolve the merge conflicts.

steveklabnik · 2015-12-31T17:06:26Z

Is this PR still active?

alexcrichton · 2016-01-11T18:28:57Z

Closing due to inactivity, but feel free to resubmit with a rebase!

mkpankov added 2 commits October 17, 2015 02:05

rustfmt the libcore/num

09a4df5

Merge branch 'master' into core-fmt

4679b0c

rust-highfive assigned aturon Oct 16, 2015

rust-highfive assigned nrc and unassigned aturon Oct 16, 2015

hanna-kruppe reviewed Oct 17, 2015
View reviewed changes

Rewrite a complex conditional

8233d7c

Manually wrap long arrays

21a8eef

hanna-kruppe mentioned this pull request Oct 17, 2015

Consider putting (slightly longer) struct literals on one line rust-lang/rustfmt#483

Closed

nrc reviewed Oct 18, 2015
View reviewed changes

mkpankov added 2 commits October 22, 2015 01:50

Merge branch 'master' into core-fmt

6cba99b

Run the new version of rustfmt

2bcf540

Restore the lost comment

53f19af

mkpankov added 2 commits October 22, 2015 02:22

Skip the generated table

a6722ee

Reformat the signature with restored comments

9bf9809

Use the proper attributes

28327e4

alexcrichton closed this Jan 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustfmt the libcore/num #29113

rustfmt the libcore/num #29113

mkpankov commented Oct 16, 2015

rust-highfive commented Oct 16, 2015

mkpankov commented Oct 16, 2015

mkpankov commented Oct 16, 2015

hanna-kruppe Oct 17, 2015

marcusklaas Oct 17, 2015

hanna-kruppe Oct 17, 2015

marcusklaas Oct 17, 2015

mkpankov Oct 17, 2015

hanna-kruppe Oct 17, 2015

hanna-kruppe commented Oct 17, 2015

mkpankov commented Oct 17, 2015

mkpankov commented Oct 17, 2015

nrc commented Oct 18, 2015

nrc commented Oct 18, 2015

nrc Oct 18, 2015

nrc commented Oct 18, 2015

nrc commented Oct 18, 2015

mkpankov commented Oct 21, 2015

mkpankov commented Oct 21, 2015

mkpankov commented Oct 21, 2015

nrc commented Oct 21, 2015

mkpankov commented Oct 22, 2015

nrc commented Oct 22, 2015

mkpankov commented Oct 22, 2015

mkpankov commented Oct 24, 2015

mkpankov commented Oct 24, 2015

marcusklaas commented Oct 24, 2015

nrc commented Oct 25, 2015

nrc commented Oct 25, 2015

bors commented Nov 1, 2015

steveklabnik commented Dec 31, 2015

alexcrichton commented Jan 11, 2016

rustfmt the libcore/num #29113

rustfmt the libcore/num #29113

Conversation

mkpankov commented Oct 16, 2015

rust-highfive commented Oct 16, 2015

mkpankov commented Oct 16, 2015

mkpankov commented Oct 16, 2015

hanna-kruppe Oct 17, 2015

Choose a reason for hiding this comment

marcusklaas Oct 17, 2015

Choose a reason for hiding this comment

hanna-kruppe Oct 17, 2015

Choose a reason for hiding this comment

marcusklaas Oct 17, 2015

Choose a reason for hiding this comment

mkpankov Oct 17, 2015

Choose a reason for hiding this comment

hanna-kruppe Oct 17, 2015

Choose a reason for hiding this comment

hanna-kruppe commented Oct 17, 2015

mkpankov commented Oct 17, 2015

mkpankov commented Oct 17, 2015

nrc commented Oct 18, 2015

nrc commented Oct 18, 2015

nrc Oct 18, 2015

Choose a reason for hiding this comment

nrc commented Oct 18, 2015

nrc commented Oct 18, 2015

mkpankov commented Oct 21, 2015

mkpankov commented Oct 21, 2015

mkpankov commented Oct 21, 2015

nrc commented Oct 21, 2015

mkpankov commented Oct 22, 2015

nrc commented Oct 22, 2015

mkpankov commented Oct 22, 2015

mkpankov commented Oct 24, 2015

mkpankov commented Oct 24, 2015

marcusklaas commented Oct 24, 2015

nrc commented Oct 25, 2015

nrc commented Oct 25, 2015

bors commented Nov 1, 2015

steveklabnik commented Dec 31, 2015

alexcrichton commented Jan 11, 2016