add more formatting traits #56

maxbla · 2019-07-22T19:08:05Z

fixes #1
TODO:

eliminate heap use for no_std (does not support padding)
- add better formatting tests for no_std
write a macro for formatting trait impls
- ~~simplify macro for formatting trait impls using format_args!~~

maxbla · 2019-07-22T19:11:35Z

~~As it stands right now, this PR is a breaking change because it makes Display only work on types that are Clone + Integer.~~ I did this so I could properly deal with both padding and negative signs, although I may have to remove that feature (for Display, but keep it for the new formatting traits) to maintain backwards compatibility

Edit: The PR properly deals with padding and negative signs for new formatting traits, but not Display, so as to maintain backwards compatibility

src/lib.rs

maxbla · 2019-07-25T09:21:27Z

I never said it, but this is intended to fix #1

maxbla · 2019-08-09T19:16:14Z

I can't figure out how to use std::format! in crates using no_std for older rust versions. When I type std::format!(), it works in new rust, but rust 1.26 and 1.15 don't work.

error[E0658]: non-ident macro paths are experimental (see issue #35896)

    --> src/lib.rs:1096:13

     |

1096 |             std::format!("{:o}", self.numer)

     |             ^^^^^^^^^^^

(see Travis for full output)
If I try to use format!(with #[macro_use] on extern crate std and use std::format) it just fails error: macro undefined: format! I could easily

cuviper · 2019-08-09T19:20:25Z

The format! macro creates a String -- that's only possible on no_std targets with the alloc crate, which was only stabilized in Rust 1.36. I'm not sure if all no_std targets are guaranteed to have alloc even then.

maxbla · 2019-08-09T19:40:20Z

I'm also setting the #[cfg(feature = "std")] attribute for the method that I'm trying to use format! in, so that should allow use of std::string::String, right? Also the code I wrote works in rust 1.31.0 and current stable according to TravisCI.

cuviper · 2019-08-09T20:01:42Z

Ah, note how std is imported:

#[cfg(feature = "std")]
#[cfg_attr(test, macro_use)]
extern crate std;

Make that a direct #[macro_use] and you should be able to use format! directly.

I think the std::format! style was added in 1.30.0, as part of macro use statements.

Macros exported with #[macro_export] are now placed into the root module of the crate.

maxbla · 2019-08-13T01:34:54Z

This PR is no longer a breaking change -- the original Display trait remains unchanged, with no support for padding. I will make a PR with the breaking changes to Display in it in the future that may be merged at the same time as other breaking changes. It may make sense to include an implementation of Display specific to no_std and one (more fully featured) that depends on std::string::String.

That said, I think this PR is ready for review.

cuviper · 2019-08-15T23:42:35Z

src/lib.rs

+        assert_eq!(&format!("{:#010b}", _1_2), "0b001/0b10");
+        let half_i8: Ratio<i8> = Ratio::new(1_i8, 2_i8);
+        assert_eq!(&format!("{:b}", -half_i8), "11111111/10");
+        assert_eq!(&format!("{:#b}", -half_i8), "0b11111111/0b10");


Ew -- I find it unfortunate that negative numbers are formatted this way, especially since they'll overflow if you try to parse it back, but I know it's not your doing. If you're curious, it comes from core's int_base! casting as unsigned.

cuviper · 2019-08-15T23:49:48Z

I should have mentioned before, num-complex goes through similar pains to format multiple numbers simultaneously, including std/no_std. One trick you might like is using format_args! into a nested function (necessary for its borrowed lifetimes) to let it work without format! strings. It's all wrapped in a write_complex! macro to be shared among the various traits.

cuviper · 2019-08-15T23:51:32Z

You don't have to match that complexity (pun!) but it may show you some useful tricks.

src/lib.rs

cuviper · 2020-02-17T21:33:21Z

I would like to include this in 0.3 (#65), and notably this means we can make the breaking change you had earlier on Display. Can you rebase this, and perhaps make that change, so I can give it another review?

src/lib.rs

TODO: eliminate heap use, write a macro for Display impls

Co-Authored-By: Josh Stone <[email protected]>

previously formatting would fail the following test case format!("{:+}", Ratio:new(-2, 1)), returning "+-2", utter nonsense. This fix adds a check for a negative sign (dash) in an intermediate string representation, removing it if found. no_std targets previously didn't support sign_plus(), but now do.

Now the correct features will be enabled when running the ci script outside of ci environments.

Use autofcg to detect if integers support exponential formatting. If so, autocfg sets the has_int_exp_fmt cfg, used in tests.

maxbla · 2020-03-19T02:05:12Z

There's one more thing this doesn't support - precision. Is it worth it? Is there a way to do it that doesn't introduce much more complexity?

cuviper · 2020-03-19T19:04:14Z

Precision would really only be useful for the Exp formats, right? I think it's fine to defer that.

cuviper

I'd like the build script to be simpler, but then I think we're ready to merge!

cuviper · 2020-03-19T21:14:03Z

build.rs

+            && ac.probe_expression("format!(\"{:e}\", 0_u64)")
+            && ac.probe_expression("format!(\"{:e}\", 0_u128)"))
+        {
+            panic!("Some integer types implement *Exp traits, but not others")


This is a bit overkill, and has a tangible effect on compile times -- I measured that running the build script on current stable takes ~110ms, whereas nightly takes ~680ms. After all, each probe requires a new rustc process invocation.

If you really want to test them all, it could be done better in one large probe expression. However, we really only care about this feature for testing purposes, so I don't think it needs to be so thorough. For that matter, the test only uses it for Rational = Ratio<isize>, which you didn't even probe... 😉

Let's just probe for support in isize and call it good.

each probe requires a new rustc process invocation

I didn't even think about how it worked, but that makes sense.

Let's just probe for support in isize and call it good.

Sounds good.

maxbla · 2020-03-20T07:25:18Z

Precision would really only be useful for the Exp formats, right?

Yep, that's right.

I think it's fine to defer that.

Agreed. I don't want to deal with it now and *Exp trait implementations for Integers hasn't landed in stable yet.

I'm considering writing an RFC about how extending formatting (as this crate does) is more difficult than it needs to be because of the api. Off the top of my head, having sign_minus mean never render a sign would be nice, and there should be some way to say default precision with the .* precision specifier (maybe by taking a type that is Into<Option<usize>>, so None works). I'm not sure how this should work, but it would also be nice if there was some way to turn a type + formatting trait + formatter into a string. i.e. not need to generate a formatting string. This could work by creating a "mock" formatter that looks like a normal formatter, but writes to a String instead of to stdout, or whatever.

cuviper · 2020-03-21T16:16:53Z

Let's ship it!

bors r+

bors · 2020-03-21T16:20:16Z

Build succeeded

continuous-integration/travis-ci/push

maxbla commented Jul 22, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

maxbla commented Jul 22, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cuviper added the breaking change label Jul 23, 2019

cuviper reviewed Aug 15, 2019

View reviewed changes

maxbla commented Dec 6, 2019

View reviewed changes

src/lib.rs Show resolved Hide resolved

cuviper mentioned this pull request Jan 10, 2020

Tracking issue for num-rational 0.3 #65

Closed

3 tasks

cuviper reviewed Feb 19, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cuviper reviewed Feb 19, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cuviper reviewed Feb 19, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cuviper reviewed Feb 19, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cuviper reviewed Mar 13, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cuviper reviewed Mar 13, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cuviper reviewed Mar 13, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

maxbla changed the title ~~WIP: add more formatting traits~~ add more formatting traits Mar 18, 2020

maxbla added 6 commits March 18, 2020 12:26

WIP: add more formatting traits

f8023db

TODO: eliminate heap use, write a macro for Display impls

make negative check more sane

b1a95e7

revert to old implementation of Display

e96d0ed

switch from alloc::format to std::alloc

c1e3ab3

fix format! macro in 1.26.0 and older rust

a3c45e3

remove negative signs from some formatting traits

ebe4b2e

maxbla and others added 19 commits March 18, 2020 12:45

add test cases

272053f

remove extra trait bounds on Display traits

49b41c2

switch formatting impls to using macros

b4b1c82

improve Exp formatting traits' tests

e582dfe

remove dependency on std for Exp formatting traits

f7aea8f

appease rustfmt

c7ea97a

attempt to support no_std

a661879

Add breaking change to Display

aa1c26b

fix imports in rust 1.31

bb3a544

fix typo

570e401

Co-Authored-By: Josh Stone <[email protected]>

maybe fix imports

37804b4

improve no_std display tests

ac41f59

add parsing rust version from rustc to ci script

c67e0ae

Now the correct features will be enabled when running the ci script outside of ci environments.

fix shellcheck suggestions on ci script

09e859a

reduce code duplication in formatting code

e985a0f

fix displaying negatives with padding

92064f3

Improve testing exponential formatting

92f8027

Use autofcg to detect if integers support exponential formatting. If so, autocfg sets the has_int_exp_fmt cfg, used in tests.

format build.rs

e0e3cb9

cuviper reviewed Mar 19, 2020

View reviewed changes

hasten builds by only probing isize *Exp formatting

05213bd

bors bot merged commit 47d63e2 into rust-num:master Mar 21, 2020

cuviper mentioned this pull request Nov 6, 2020

Fix always rebuilding with --remap-path-prefix #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more formatting traits #56

add more formatting traits #56

maxbla commented Jul 22, 2019 •

edited

Loading

maxbla commented Jul 22, 2019 •

edited

Loading

maxbla commented Jul 25, 2019

maxbla commented Aug 9, 2019

cuviper commented Aug 9, 2019

maxbla commented Aug 9, 2019

cuviper commented Aug 9, 2019

maxbla commented Aug 13, 2019

cuviper Aug 15, 2019

cuviper commented Aug 15, 2019

cuviper commented Aug 15, 2019

cuviper commented Feb 17, 2020

maxbla commented Mar 19, 2020

cuviper commented Mar 19, 2020

cuviper left a comment

cuviper Mar 19, 2020

maxbla Mar 20, 2020

maxbla commented Mar 20, 2020

cuviper commented Mar 21, 2020

bors bot commented Mar 21, 2020

add more formatting traits #56

add more formatting traits #56

Conversation

maxbla commented Jul 22, 2019 • edited Loading

maxbla commented Jul 22, 2019 • edited Loading

maxbla commented Jul 25, 2019

maxbla commented Aug 9, 2019

cuviper commented Aug 9, 2019

maxbla commented Aug 9, 2019

cuviper commented Aug 9, 2019

maxbla commented Aug 13, 2019

cuviper Aug 15, 2019

Choose a reason for hiding this comment

cuviper commented Aug 15, 2019

cuviper commented Aug 15, 2019

cuviper commented Feb 17, 2020

maxbla commented Mar 19, 2020

cuviper commented Mar 19, 2020

cuviper left a comment

Choose a reason for hiding this comment

cuviper Mar 19, 2020

Choose a reason for hiding this comment

maxbla Mar 20, 2020

Choose a reason for hiding this comment

maxbla commented Mar 20, 2020

cuviper commented Mar 21, 2020

bors bot commented Mar 21, 2020

Build succeeded

maxbla commented Jul 22, 2019 •

edited

Loading

maxbla commented Jul 22, 2019 •

edited

Loading