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.

Sign up for GitHub

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

Jump to bottom

Remove or document Unsafe.AsPointer uses in core libraries #99146

Merged
merged 14 commits into from
Mar 5, 2024
Merged

Remove or document Unsafe.AsPointer uses in core libraries #99146

merged 14 commits into from
Mar 5, 2024

Conversation

hamarb123
Copy link
Contributor

Contributes to #99144

@hamarb123
Remove or document Unsafe.AsPointer uses in System.Memory
6e4a388 
- Also add internal helper method Unsafe.IsOpportunisticallyAligned
- And cast a pointer to IntPtr instead of int for a unit test
@ghost
Copy link

ghost commented Mar 1, 2024

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #99144

Author: hamarb123
Assignees: -
Labels:

area-System.Memory
Milestone: -

@jkotas
Copy link
Member

jkotas commented Mar 1, 2024

Many tests are failing with

Should've been aligned on natural word boundary.
   at System.SpanHelpers.ClearWithReferences(IntPtr& ip, UIntPtr pointerSizeLength) in /_/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.cs:line 17

hamarb123 added 2 commits March 2, 2024 07:05
@hamarb123
Address feedback
9d52e37
@hamarb123
Changes for the rest of the core libraries
a40d752 
- Other than MethodHandle code for NativeAOT
- Add Unsafe.OpportunisticMisalignment
@hamarb123 hamarb123 changed the title Remove or document Unsafe.AsPointer uses in System.Memory Remove or document Unsafe.AsPointer uses in core libraries Mar 4, 2024
@hamarb123
Copy link
Contributor Author

hamarb123 commented Mar 4, 2024 •
edited
Loading

I've skipped the code in NativeAOT's MethodHandle files, and QCallHandles (out of the core libraries)

@build-analysis build-analysis bot mentioned this pull request Mar 4, 2024
Closed
@hamarb123
Copy link
Contributor Author

Should I include changes for the misc tests in this PR too?

@jkotas
Copy link
Member

jkotas commented Mar 4, 2024

Should I include changes for the misc tests in this PR too?

I would start a new PR for these.

jkotas
jkotas approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jkotas jkotas mentioned this pull request Mar 5, 2024
Closed
@jkotas jkotas merged commit 825bc44 into dotnet:main Mar 5, 2024
188 of 190 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalPetryka MichalPetryka MichalPetryka left review comments

@tannergooding tannergooding tannergooding left review comments

@huoyaoyuan huoyaoyuan huoyaoyuan left review comments

@sharwell sharwell sharwell left review comments

@EgorBo EgorBo EgorBo left review comments

@jkotas jkotas jkotas approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees
No one assigned
Labels
area-System.Memory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hamarb123 @jkotas @EgorBo @sharwell @huoyaoyuan @tannergooding @MichalPetryka