Simplify format_integer_with_underscore_sep

[yotamofek]

Noticed that this helper fn only ever gets called with decimal-base-formatted ints, so can be simplified a lot by not trying to handle hex and octal radixes.
Second commit is completely unrelated, just simplified some code I wrote a while back 😁

[rustbot]

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[lolbinarycat]

a few issues but mainly seems like a decent idea to slim down an overly flexible test helper.

[lolbinarycat]

using as_bytes and char::from like this is technically a logic error, as it will turn every byte of a UTF-8 sequence into the corresponding unicode codepoint.

it does happen to work here since we're never using non-ascii (if we were then reversing a string bytewise would be extremely problematic), but String::from_utf8(...).unwrap() represents the intent much more cleanly.

[yotamofek]

Not that this function is hot enough to prefer performance over correctness,
but I think in this case it's quite obvious that this is ok since Displaying integers will always result in an ASCII-only string. We can also collect into a vector of chars, like the old version.

[lolbinarycat]

If we do want to care about performance here, I would just use String::from_utf8_unchecked. This should actually be more performant than char::from since it fully erases any non-ascii code. it will also at least give us a panic on miri instead of just silent corruption if somehow this gets rewritten into something that does produce non-ascii.

there's also u8.as_ascii(), since String impls Extend<ascii::Char>.

[notriddle]

There's no way unsafe code would be justified here.

as_ascii seems like the right way to do this, since it panics if it's given non-ASCII text, but doesn't make the code much more complex:

fn format_integer_with_underscore_sep(num: u128, is_negative: bool) -> String {
    let num = num.to_string();
    let mut result = if is_negative { "-" } else { "" }.to_string();
    result.extend(
        num.as_bytes().rchunks(3).rev().intersperse(b"_").flatten().copied().map(|b| b.as_ascii().unwrap())
    );
    result
}

[lolbinarycat]

If we wouldn't accept unsafe here, then I think we definitely shouldn't accept the potential of silent logic errors.

One nice thing is that String::from_utf8 has a concrete signature, so you can use collect without type hints, but I'm really fine with any solution which isn't char::from. If nothing else using char::from in this way is a bad habit I don't think we should encourage.

[yotamofek]

Ended up using str::as_ascii so there's only one unwrap, IMHO it looks kinda nice now!

[yotamofek]

a few issues but mainly seems like a decent idea to slim down an overly flexible test helper.

BTW, this is not a test helper, it's used in the generation of docs :)

[lolbinarycat]

BTW, this is not a test helper, it's used in the generation of docs :)

yeah i noticed that after your comment, github put the diff break in a very confusing spot :p

[notriddle]

@bors r+ rollup

[bors]

📌 Commit 5c735d1 has been approved by notriddle

It is now in the queue for this repository.