Github stabilize `int_error_matching` by eopb · Pull Request #84910 · rust-lang/...

Copy link

Contributor

rust-highfive commented on May 4

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

This comment has been hidden.

joshtriplett commented on May 4

Posted one comment about not stabilizing __description. Other than that, this looks good to me.

r? @joshtriplett for the T-libs FCP and since you've already started a review here

rfcbot commented 28 days ago •

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

bors commented 27 days ago

The latest upstream changes (presumably #85014) made this pull request unmergeable. Please resolve the merge conflicts.

This comment has been hidden.

This comment has been hidden.

bors commented 26 days ago

The latest upstream changes (presumably #85058) made this pull request unmergeable. Please resolve the merge conflicts.

gilescope commented 23 days ago

You're going to hate me, but I got a snag. https://godbolt.org/z/fPx6ThTYx
It seems that the performance difference between one and two enum arms is very different for u128s. One arm takes 1.7ns but two arms takes 4.7ns.

I would love to understand what's going on because the error is clearly smaller than a u128 even with a fair few enum varients. I don't know why llvm would generate different assembly. I take it mov is much slower than an xor but why llvm why?

gilescope commented 23 days ago

(I know worrying about 3ms when the largest i128 currently takes 300ns to parse seems pedantic but I'm working on getting parsing 128s to below 20ns worstcase so it's becoming not insignificant.)

gilescope commented 23 days ago

Hmm, seems like our side looking at the generated llvm instructions, but I'm not great at reading the runes.

rfcbot commented 20 days ago

This is now entering its final comment period, as per the review above.

gilescope commented 20 days ago

Think the bot might have jumped the gun here.

kennytm commented 20 days ago

I see 5 out of 7 boxes, which allows FCP to commence. The bot is working fine.

gilescope commented 18 days ago

I have concerns that the code gen could perhaps be faster, but that's a seperate concern to stabilisation. Stabilise away.

yaahc commented 16 days ago •

I have concerns that the code gen could perhaps be faster, but that's a seperate concern to stabilisation. Stabilise away.

i'm not really sure I understand the code gen concern. Is the issue that match statements against enums with multiple variants are slower or that using an error with multiple variant's changes the layout of a Result<u128, TheError> type? Either way, I don't really see a way to resolve this, we already use these patterns all throughout the rest of std.

Also, I did a quick test and it seems like it might be possible to avoid the issue by wrapping the inner method we export in std with another one that drops the error type so that the Result can be optimized the same way it would in the fast case you showed.

https://godbolt.org/z/MzPT8rMdM

This comment has been hidden.

gilescope commented 16 days ago

Wow. Compilers are so cool. That's very impressive. - thanks @yaahc

rfcbot commented 10 days ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

bors commented 9 days ago

The latest upstream changes (presumably #84985) made this pull request unmergeable. Please resolve the merge conflicts.