Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default impl of PartialOrd for VecN is a footgun #168

Closed
sanbox-irl opened this issue Apr 24, 2021 · 4 comments
Closed

Default impl of PartialOrd for VecN is a footgun #168

sanbox-irl opened this issue Apr 24, 2021 · 4 comments

Comments

@sanbox-irl
Copy link
Contributor

Hello!

I am re-raising the issue in #138

I had some code that had x > y, by which I actually meant x.cmpgt(y).all(), which I had mistakenly assumed the default PartialOrd impl was.

When my unit tests (ahem...which I totally had...and didn't add after noticing the bug...ahem) failed, it got me thinking about this implementation -- and specifically what it actually was. It's not obvious what the implementation is looking at it. I'm very familiar with this library as a user but know very little about how the macros work in it, so I actually just couldn't find the core implementation of PartialOrd after a bit of searching around (certainly RA wasn't any help).

I think one of two things should happen:

  1. The PartialOrd implementations should be removed.
  2. The PartialOrd implementations should have some semi-obvious documentation notes about them (perhaps even linked in the .cmp functions).

Additionally, I should avoid programming late at night. If any of those three things could happen, this small bug would have been avoided.

Thank you for all your hard work on glam! It's excellent

@bitshifter
Copy link
Owner

That is fair. I mostly added it because it's implemented for (f32, f32, f32) and [f32; 3], in fact the glam implementation is casting to [f32,3] to get PartialOrd. You aren't the first person to mention it though, so perhaps it is time to nerf it.

The macros have also come up a few times as making it hard for people to see what the library is doing now. It is a bit unfortunate, I would like to make the code readable to people other than me again one day.

@sanbox-irl
Copy link
Contributor Author

Mhm, I'm not sure what can be done to help with the macros. Can the new doc-alias feature help?

@bitshifter
Copy link
Owner

Removed all PartialOrd and Ord trait impls.

@sanbox-irl
Copy link
Contributor Author

ty!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants