search

rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.37k stars 337 forks source link

Refactor float casting tests, add `f16` and `f128` #3688

Closed tgross35 closed 2 months ago

tgross35 commented 3 months ago

This is an attempt to remove the magic from a lot of the numbers tested, which makes things easier when adding f16 and f128. A nice side effect is that these tests now cover all int <-> float conversions with the same amount of tests.

tgross35 commented 2 months ago

Ah I thought https://github.com/rust-lang/miri/pull/3721 might have gotten it, but it looks like https://github.com/rust-lang/rust/pull/127032 still didn't get synced yet.

RalfJung commented 2 months ago

I did another rustup, if you rebase again it should work.

tgross35 commented 2 months ago

Thanks for the update. Great news is that most of these pass, but I need to fix how I restrict a couple of them. And wrap up the remaining comments of course.

tgross35 commented 2 months ago

🎉 with a better understanding of the tests in https://github.com/rust-lang/miri/pull/3688#discussion_r1664895098, everything seems to be passing for all four float types. Ready for a (hopefully) final look.

@rustbot ready

RalfJung commented 2 months ago

In future please avoid rebases unless there are conflicts or other reasons that require a rebase... it seems I now have no way to see the diff of what you changed since my earlier review. :/

RalfJung commented 2 months ago

All right, I think we're good to go. Thanks for your patience through all these reviews!

Please squash the commits (ideally in a way that the force push has no diff).

tgross35 commented 2 months ago

In future please avoid rebases unless there are conflicts or other reasons that require a rebase... it seems I now have no way to see the diff of what you changed since my earlier review. :/

Do you mean a squashes or specifically rebase onto latest? I'm tend to aggressively squash / fixup to keep commit history clean, because I commit frequently and otherwise sometimes things get merged with a messy log (drives me crazy...). The actual change of base wasn't intentional, I just did it from a different checkout (need to get in the habit of merge-base, but I can never remember the exact incantation to get things right).

In any case the change from my previous push is in https://github.com/rust-lang/miri/compare/dfddc6856e10ecf25f9808d83f5818430fc0ce67..bbfe57710ce7072ef7417188f2d3b1891382ea01#diff-64fd96f1f6fb00d3a7940a8e5c347ce262ea751844538a2c87d4b17574a01f51. Wish GH was just better hiding change-of-base differences.

So I'll just wait until you're ready and then squash one last time 😆

(edit: oh that's now)

tgross35 commented 2 months ago

Okiedoke, squashed. Thanks for all the reviews!

RalfJung commented 2 months ago

Force-pushing during review in general isn't great since github tends to lose diff then, but change-of-base makes it a lot worse. If you commit frequently you can still squash the new commits before you push to avoid force-pushes. :D

@bors r+

bors commented 2 months ago

:pushpin: Commit 561cd7317cb49a28f8ae56e51a989e6f4139244b has been approved by RalfJung

It is now in the queue for this repository.

bors commented 2 months ago

:hourglass: Testing commit 561cd7317cb49a28f8ae56e51a989e6f4139244b with merge ddbbecb2369474924c4ba415b56d0f7eacff097c...

bors commented 2 months ago

:sunny: Test successful - checks-actions Approved by: RalfJung Pushing ddbbecb2369474924c4ba415b56d0f7eacff097c to master...