Remove unreachable #8
Conversation
The old API exposes `unreachable` in both unescape_unicode and unescape_mixed. These are conceptually one function, but because their return types are incompatible, they could not be unified. The new API takes this insight further to separate unescape_unicode into separate functions, such that byte functions can return bytes instead of chars.
…remove unused Mode methods
rust-cloud-vms
bot
force-pushed
the
remove_unreachable
branch
from
f816e0b to
00b6cfd
Compare
|
Gonna take a look later on. |
|
Thanks for splitting this into multiple pieces. I think it's good that @GuillaumeGomez will look at this, it will be good to get another pair of eyes on it after rust-lang/rust#138163. |
|
Changes look good to me. Now comes the not so fun question: can you add benchmarks please? |
Are you thinking about numbers (before the crate split off this was looking like this for the macro variant of this change) or code for this crate? I'm happy to come up with some benchmark code. |
|
Mostly code, I can check locally when done. Considering it'll impact performance, better check ahead of time. We just need to check the entry functions. |
|
Benchmarks PR: #9 |
| /// Takes the contents of a literal (without quotes) | ||
| /// and produces a sequence of errors, | ||
| /// which are returned by invoking `error_callback`. | ||
| pub fn unescape_for_errors( |
There was a problem hiding this comment.
What is the purpose of this function? It does the conversion but doesn't actually make use of it. It only provides information if one error occurred. Where is it meant to be used?
|
Also need to update the benchmarks. |
This improves the API of this crate to not use
unreachableany more and is the continuation of rust-lang/rust#138163.It also eliminates internal
unreachableby inliningModemethods into the *_common functions, and eliminates the resulting duplication by using traits instead. Using traits is much more verbose than a macro-based variant, because they are very explicit, but hopefully also a bit less unclear.I've tried to use separate commits to explain the story, but have probably only succeeded at the beginning.
There is a companion PR to use this new API here: rust-lang/rust#140999
r? @nnethercote