-
Notifications
You must be signed in to change notification settings - Fork 215
Add RequiresUnsafeAttribute and start annotating corelib #3196
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
base: feature/requires-unsafe
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,10 @@ public override T[] Rent(int minimumLength) | |
| SharedArrayPoolThreadLocalArray[]? tlsBuckets = t_tlsBuckets; | ||
| if (tlsBuckets is not null && (uint)bucketIndex < (uint)tlsBuckets.Length) | ||
| { | ||
| buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array); | ||
| unsafe | ||
| { | ||
| buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good case-study for the annotation guidelines. This is safe because we manually ensure that we only assign |
||
| } | ||
| if (buffer is not null) | ||
| { | ||
| tlsBuckets[bucketIndex].Array = null; | ||
|
|
@@ -79,7 +82,10 @@ public override T[] Rent(int minimumLength) | |
| SharedArrayPoolPartitions? b = perCoreBuckets[bucketIndex]; | ||
| if (b is not null) | ||
| { | ||
| buffer = Unsafe.As<T[]>(b.TryPop()); | ||
| unsafe | ||
| { | ||
| buffer = Unsafe.As<T[]>(b.TryPop()); | ||
| } | ||
| if (buffer is not null) | ||
| { | ||
| if (log.IsEnabled()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,9 +128,15 @@ public TDelegate Current | |
| public bool MoveNext() | ||
| { | ||
| int index = _index + 1; | ||
| if ((_current = Unsafe.As<TDelegate>(_delegate?.TryGetAt(index))) == null) | ||
| unsafe | ||
| { | ||
| return false; | ||
| unsafe | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is really, really unsafe I guess :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol not sure what happened there |
||
| { | ||
| if ((_current = Unsafe.As<TDelegate>(_delegate?.TryGetAt(index))) == null) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| _index = index; | ||
| return true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace System.Diagnostics.CodeAnalysis | ||
| { | ||
| /// <summary> | ||
| /// Indicates that the specified method requires dynamic access to code that is not referenced | ||
| /// statically, for example through <see cref="Reflection"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This allows tools to understand which methods are unsafe to call when removing unreferenced | ||
| /// code from an application. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)] | ||
| internal sealed class RequiresUnsafeAttribute : Attribute | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we know some version of this attribute is going to be required, should we go ahead and take it to API review so we can land on the name (even if the exact constructors it contains may change over time)? There's been a few names that have been floated around such as |
||
| { | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove the unsafe here and just cast to
T[]here, right?The only reason this isn't already
T[]and that it's usingSystem.Arrayinstead is to avoid the generic code size increase for theSharedArrayPoolThreadLocalArraystruct, but maybe that's something we could track addressing as well if the cast is too expensive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this has perf fine-tuned using unsafe code. Number of other cases in this PR are in the same category.