Optimize core::unicode::printable.

[reitermarkus]

Follow-up to #138024, which apparently still wasn't enough to fully optimize char::escape_debug.

Again, this only seems to make a difference on embedded targets, e.g. thumbv7m-none-eabi.

[rustbot]

r? @workingjubilee

rustbot has assigned @workingjubilee.
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

[reitermarkus]

r? libs

[reitermarkus]

r? libs

[reitermarkus]

r? libs

[jhpratt]

Not sure if he's a libs reviewer, but Manish is probably the best person to review anything Unicode.

r? @Manishearth

[Manishearth]

This appears to be an optimization. It doesn't seem to be doing anything super Unicodey; it appears to be doing some byte lookup optimizations. I don't think I have the bandwidth to review this right now.

I'm wary about the unsafe here. If this is going to have unsafe, it should be thoroughly documented. At the moment there's no clarity on what these functions or these constants actually are, which isn't great but it's "not great" in an okay way because there's no unsafe.

I'd also suggest rewriting it so that the actual Rust code does not need to be autogenerated, just some constants and a small match for the large parts.

[reitermarkus]

r? @jhpratt

[jhpratt]

I really am not familiar with this kind of code; I passed off for a reason.

r? libs

[reitermarkus]

@jhpratt, to be fair, I am also only slightly familiar with the code. 😄

I know what it does, but not how the underlying Unicode logic works. This PR doesn't change the logic, it only refactors the code to make it easier to reason about why the bounds checks are unneeded.

[reitermarkus]

r? libs

[Mark-Simulacrum]

Do we have evidence that removing this unsafe has meaningful impact on performance or code size? I wouldn't expect the bounds checks to matter that much.

[reitermarkus]

@Mark-Simulacrum, the problem here isn't performance or code size, since on most targets, it is already optimized. The problem is that on some embedded targets, e.g. thumbv7m-none-eabi, the bounds check panic isn't optimized out in release mode, making it impossible to use panic-never or no_panic to ensure there are no reachable panics.

[Mark-Simulacrum]

Can you say more? Why do only some targets see the panics stay in?

In general, I don't think the standard library optimizes for "unreachable panics should not appear in the binary", and I'm reluctant to do so in this sort of code unless we gate that on e.g. a test-only assert and have exhaustive proof that it never triggers (e.g., by running it on every possible char).

[reitermarkus]

@rustbot ready