-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Implement int_format_into
feature
#142098
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
Conversation
Failed to set assignee to
|
assert_eq!(ret, value); | ||
|
||
let mut buffer = NumBuffer::<$int>::new(); | ||
assert_eq!(value.format_into(&mut buffer), s.as_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since format_into
is not part of a trait, I needed to switch from a function to a macro to avoid the generics limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could write and implement a trait right here, in the test - right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could indeed, but would be better than the current code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less churn; other than that I don't have strong feelings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone else agrees with you I'll make the change (lazyness++ 😆).
r? @Amanieu |
This comment has been minimized.
This comment has been minimized.
1d9a06e
to
2589402
Compare
d11cb44
to
70ab4e0
Compare
This comment has been minimized.
This comment has been minimized.
70ab4e0
to
3eecb24
Compare
3eecb24
to
2f869c5
Compare
Applied suggestions and opened another PR for the number of digits of u128 computation to not add even more changes to this PR. |
2f869c5
to
9324432
Compare
☔ The latest upstream changes (presumably #142133) made this pull request unmergeable. Please resolve the merge conflicts. |
9324432
to
4733100
Compare
Fixed merge conflict. |
4733100
to
61cc5f3
Compare
Maybe you want to take a look at this one too @tgross35 ? :) |
61cc5f3
to
e5c6e02
Compare
Safety annotation needs to be right on the |
This comment has been minimized.
This comment has been minimized.
a8b169e
to
9e76d15
Compare
Yeah I will just revert the |
9e76d15
to
fb17077
Compare
And done. |
/// Initializes internal buffer. | ||
#[unstable(feature = "int_format_into", issue = "138215")] | ||
pub const fn new() -> Self { | ||
// FIXME: Once const generics feature is working, use `T::BUF_SIZE` instead of 40. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a const { assert!(T::BUF_SIZE <= 40) }
here.
/// Returns the length of the buffer. | ||
#[unstable(feature = "int_format_into", issue = "138215")] | ||
pub const fn len(&self) -> usize { | ||
self.buf.len() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe return T::BUF_SIZE
here for forward-compatiblity.
library/core/src/fmt/num.rs
Outdated
@@ -248,6 +253,13 @@ macro_rules! impl_Display { | |||
issue = "none" | |||
)] | |||
pub fn _fmt<'a>(self, buf: &'a mut [MaybeUninit::<u8>]) -> &'a str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be an unsafe function because it makes assumptions on the size of buf
.
library/core/src/fmt/num.rs
Outdated
n /= 10; | ||
|
||
if n == 0 { | ||
break; | ||
} | ||
} | ||
} | ||
cur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be curr
? How does this even compile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's because of optimize_for_size
. I'm surprised it's not tested in CI.
library/core/src/fmt/num.rs
Outdated
@@ -336,20 +421,25 @@ macro_rules! impl_Display { | |||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsafe is not needed here since get_unchecked
is not used.
library/core/src/fmt/num.rs
Outdated
@@ -598,12 +688,20 @@ impl u128 { | |||
issue = "none" | |||
)] | |||
pub fn _fmt<'a>(self, buf: &'a mut [MaybeUninit<u8>]) -> &'a str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this needs to be unsafe because it assumes the slice size. Or _fmt_inner
should be made safe.
…n-128-integers, r=tgross35 Use a distinct `ToString` implementation for `u128` and `i128` Part of rust-lang#135543. Follow-up of rust-lang#136264. When working on rust-lang#142098, I realized that `i128` and `u128` could also benefit from a distinct `ToString` implementation so here it. The last commit is just me realizing that I forgot to add the format tests for `usize` and `isize`. Here is the bench comparison: | bench name | last nightly | with this PR | diff | |-|-|-|-| | bench_i128 | 29.25 ns/iter (+/- 0.66) | 17.52 ns/iter (+/- 0.7) | -40.1% | | bench_u128 | 34.06 ns/iter (+/- 0.21) | 16.1 ns/iter (+/- 0.6) | -52.7% | I used this code to test: ```rust #![feature(test)] extern crate test; use test::{Bencher, black_box}; #[inline(always)] fn convert_to_string<T: ToString>(n: T) -> String { n.to_string() } macro_rules! decl_benches { ($($name:ident: $ty:ident,)+) => { $( #[bench] fn $name(c: &mut Bencher) { c.iter(|| convert_to_string(black_box({ let nb: $ty = 20; nb }))); } )+ } } decl_benches! { bench_u128: u128, bench_i128: i128, } ```
fb17077
to
9943c19
Compare
Applied suggestions. |
@bors r+ |
Rollup of 7 pull requests Successful merges: - #142098 (Implement `int_format_into` feature) - #143567 (Point to correct argument in Func Call when Self type fails trait bound) - #143570 (consider nested cases for duplicate RPITIT) - #143571 (remove `has_nested` from builtin candidates) - #143586 (Fix wrong cache event query key) - #143589 (const-block-as-pattern: do not refer to no-longer-existing nightly feature) - #143608 (Fix in std::String docs) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #142098 - GuillaumeGomez:int_format_into, r=Amanieu Implement `int_format_into` feature I took over #138338 with `@madhav-madhusoodanan's` approval. Since #136264, a lot of changes happened so I made use of them to reduce the number of changes. ACP approval: rust-lang/libs-team#546 (comment) ## Associated Issue - #138215 r? `@hanna-kruppe`
I took over #138338 with @madhav-madhusoodanan's approval.
Since #136264, a lot of changes happened so I made use of them to reduce the number of changes.
ACP approval: rust-lang/libs-team#546 (comment)
Associated Issue
r? @hanna-kruppe