search

ykjit / yk

yk packages
https://ykjit.github.io/yk/
Other
28 stars 7 forks source link

Add `Float`s to `enum Const`. #1267

Closed ltratt closed 3 weeks ago

ltratt commented 3 weeks ago

This makes clear that Const can't be Eq because floats aren't. We therefore implement a new wrapper that is solely used for storing Consts in an IndexSet, without giving users the idea that Const can be compared for equailty.

Note that this doesn't fully implement all the necessary bits, but it does enough that it should make clear what extra bits need doing.

vext01 commented 3 weeks ago

https://github.com/ykjit/yk/pull/1267/commits/e327e47a04d0d8e124ab32dff3f4fd05ed446f01 implements the rest of what is needed for float consts.

Requires https://github.com/ykjit/ykllvm/pull/177

ltratt commented 3 weeks ago

Lukas is away so we'll have to break our normal rule and I'll be the reviewer for your code, and you can review mine. I'm fine with your code; if so, please squash (but you should probably leave my commit as-is).

vext01 commented 3 weeks ago

Squashed.

vext01 commented 3 weeks ago

(we have to wait for the ykllvm change to merge)

vext01 commented 3 weeks ago

Happy with https://github.com/ykjit/yk/pull/1267/commits/2596fae3d0ba40821531725f220f80680248b64b ?

ltratt commented 3 weeks ago

I'm not happy with it. I'm ecstatic! I didn't know there was a non-unsafe way doing this, and I'm happy you are less ignorant than I am! Please squash.

vext01 commented 3 weeks ago

Well, clippy caught it :P I only learned about it today!

vext01 commented 3 weeks ago

Squashed, but, hah, partially verified commit.

If that bothers you, you can resign that and force push.

Otherwise ready to merge.

ltratt commented 3 weeks ago

Kinda makes sense TBH.