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

Replace RuntimeHelpers.CreateSpan<T>(LdMemberToken) with new T[] { } #3380

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

Conversation

ds5678
Copy link

@ds5678 ds5678 commented Jan 24, 2025

Problem

Resolves #3379

Solution

  • Any comments on the approach taken, its consistency with surrounding code, etc.
    • I tried to make it very similar to the code for Span<T>(void*, int).
  • Which part of this PR is most in need of attention/improvement?
    • Testing. I'm happy to write additional tests and modify my existing test.
  • At least one test covering the code changed

Comment on lines +378 to +389
#if !NET40 && ROSLYN
public static ReadOnlySpan<byte> ReadOnlySpanInitializer_ByteArray()
{
return new byte[] { 1, 2, 3 };
}

public static ReadOnlySpan<int> ReadOnlySpanInitializer_Int32Array()
{
return new int[] { 1, 2, 3 };
}
#endif

Copy link
Author

Choose a reason for hiding this comment

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

Should I have put these in a new file and modified the options? It currently fails for Roslyn v1.

@@ -143,6 +143,40 @@ internal static bool TransformSpanTArrayInitialization(NewObj inst, StatementTra
return false;
}

internal static bool TransformRuntimeHelpersCreateSpanInitialization(Call inst, StatementTransformContext context, out ILInstruction replacement)
Copy link
Author

Choose a reason for hiding this comment

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

Thoughts on the method name?

Comment on lines +162 to +174
if (context.Settings.Utf8StringLiterals &&
elementType.IsKnownType(KnownTypeCode.Byte) &&
DecodeUTF8String(initialValue, size, out string text))
{
replacement = new LdStrUtf8(text);
return true;
}
if (DecodeArrayInitializer(elementType, initialValue, new[] { size }, valuesList))
{
var tempStore = context.Function.RegisterVariable(VariableKind.InitializerTarget, new ArrayType(context.TypeSystem, elementType));
replacement = BlockFromInitializer(tempStore, elementType, new[] { size }, valuesList.ToArray());
return true;
}
Copy link
Author

Choose a reason for hiding this comment

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

This is the same as for Span<T>(void*, int) handler. Should split this out into a shared function?

field = default;
elementType = null;
IType type = inst.Method.DeclaringType;
if (type.Namespace != "System.Runtime.CompilerServices" || type.Name != "RuntimeHelpers" || type.TypeParameterCount != 0)
Copy link
Author

Choose a reason for hiding this comment

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

Is this the right way to check? Should I have created a "known" type?

Comment on lines +231 to +235
if (inst.Arguments.Count != 1)
return false;
IMethod method = inst.Method;
if (method.Name != "CreateSpan" || method.TypeArguments.Count != 1)
return false;
Copy link
Author

Choose a reason for hiding this comment

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

Should I be checking the full method signature? I noticed that the other handler doesn't check argument types.

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.

Static Initialization for Non-Byte Spans
1 participant