Add RequiresUnsafeAttribute and start annotating corelib #3196
Conversation
| unsafe | ||
| { | ||
| return false; | ||
| unsafe |
There was a problem hiding this comment.
This one is really, really unsafe I guess :)
There was a problem hiding this comment.
lol not sure what happened there
| /// code from an application. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)] | ||
| internal sealed class RequiresUnsafeAttribute : Attribute |
There was a problem hiding this comment.
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 RequiresUnsafe (closest prior is RequiresUnreferencedCode) or UnsafeCallersOnly (closest prior is UnmanagedCallersOnly). There may be other ideas/opinions that more correctly conveys "this member is unsafe" as well.
| buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array); | ||
| unsafe | ||
| { | ||
| buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array); |
There was a problem hiding this comment.
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 using System.Array instead is to avoid the generic code size increase for the SharedArrayPoolThreadLocalArray struct, but maybe that's something we could track addressing as well if the cast is too expensive.
There was a problem hiding this comment.
Right, this has perf fine-tuned using unsafe code. Number of other cases in this PR are in the same category.
| buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array); | ||
| unsafe | ||
| { | ||
| buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array); |
There was a problem hiding this comment.
This is a good case-study for the annotation guidelines. This is safe because we manually ensure that we only assign T[] into Array field. Should the Array field and all places that assign to it be marked unsafe as well?
4 participants
No description provided.