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

VS2010 Compilation errors and Equality/HashCode fixes #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CodesInChaos
Copy link

When compiling Quaternion.cs with VS2010 I get an error because the out parameter is not definitely assigned. This is because you only assign properties. I've added result = new Quaternion() where necessary.
I've added code fixing the quality problems in safe mode for Vector2, Vector3, Vector4 and Quaternion and added a unit tests to verify if equality behaves as expected.

@mhutch
Copy link
Owner

mhutch commented Jun 22, 2011

The arg-less ctor will result in setting all fields to zero, so there will actually be 6 assignments. While a good optimizing compiler should be able to inline and eliminate it, I'm pretty sure neither .NET or Mono's JIT will do that. A better solution would be to make the fields internal and set them directly, like I did on the Vector classes.

@mhutch
Copy link
Owner

mhutch commented Jun 22, 2011

Though in practice it might just allocate everything from zeroed memory and thus be able to completely eliminate the blank ctor, AFAIK that isn't guaranteed.

@CodesInChaos
Copy link
Author

You could also simply call the constructor that takes parameters. What I did was the smallest change to actually make it compile.

@CodesInChaos
Copy link
Author

I've added code fixing the quality problems in safe mode for Vector2, Vector3, Vector4 and Quaternion and added a unit tests to verify if equality behaves as expected.

I assume the SIMD mode will fail these tests, but since I don't use mono I can't verify/fix it.

Base automatically changed from master to main March 4, 2021 19:12
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

Successfully merging this pull request may close these issues.

2 participants