Removing the Vector*_Intrinsics.cs file, regions from the numerics code, and normalizing the member order #41898
Conversation
…de, and normalizing the member order
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
| /// A structure encapsulating a four-dimensional vector (x,y,z,w), | ||
| /// which is used to efficiently rotate an object about the (x,y,z) vector by the angle theta, where w = cos(theta/2). | ||
| /// </summary> | ||
| /// <summary>A structure encapsulating a four-dimensional vector (x,y,z,w), which is used to efficiently rotate an object about the (x,y,z) vector by the angle theta, where w = cos(theta/2).</summary> |
There was a problem hiding this comment.
I'm not sure this is more readable since now the reader has to horizontally scroll to read the whole thing.
There was a problem hiding this comment.
We don't consistently cap line length anywhere else and these docs are hardly used. I don't even think they match what shows in current IntelliSense.
There was a problem hiding this comment.
(and they definitely don't match what we show in the docs: https://docs.microsoft.com/en-us/dotnet/api/system.numerics.quaternion?view=netcore-3.1)
There was a problem hiding this comment.
Why make it worse? At least before this change a reader didn't need to scroll. This change provides negative value IMO.
There was a problem hiding this comment.
I find it makes it better as these docs aren't relevant and it involves less scrolling to get to the bits you actually care about, the code.
It might be nice to either remove these docs (as they are massively out of date now anyways) or actually fix them to match what we have in the docs repo.
There was a problem hiding this comment.
@eerhardt, would you have a concern if I just removed the docs as part of this?
There was a problem hiding this comment.
If the docs are wrong or don't add value, then I wouldn't have any concern removing them.
However, in this case I think this summary is adding value, as most people (myself included) don't know what a Quaternion is off the top of their head.
There was a problem hiding this comment.
I changed the summary to Represents a vector that is used to encode three-dimensional physical rotations., which matches the current docs and is shorter so the scrolling shouldn't be a concern.
| public readonly bool IsIdentity | ||
| { | ||
| get | ||
| { | ||
| return M11 == 1f && M22 == 1f && // Check diagonal element first for early out. |
There was a problem hiding this comment.
Is the checking of the diagonal elements first no longer an important optimization?
There was a problem hiding this comment.
operator == is itself doing a diagonal element first check.
The eventual vectorized version will just do two comparisons anyways and so at that point it won't matter.
ghost
locked as resolved and limited conversation to collaborators
This does some cleanup on the System.Numerics code to make it more maintainable and hopefully simplify diffs for some upcoming changes to intrinsify more of the code.
Namely, it merges the Vector*_Intrinsics.cs file back into the Vector*.cs file as this separation doesn't strictly make sense anymore.
The _Intrinsic.cs file used to contain just methods which were intrinsic or delegated to intrinsic methods, however, with the current JIT codebase that line isn't very clearly drawn.
It also removes the use of
#regionthroughout the code. We don't typically use regions elsewhere in the runtime repo and so this normalizes the convention.Finally, it normalizes the member order so that rather than having members somewhat randomly, they are ordered as
fields,constructors,properties,operators,static methods,methods; which is roughly the general order many other files in the runtime repo follow.Members within a particular group; barring fields; are alphabetically ordered.
This should contain no functional changes modulo a couple locations where, for example,
static Matrix4x4 Add()was changed to forward tooperator +, rather than simply duplicating the logic.