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

Use ReadOnlySpan<T> instead of byte[] #9

Open
bruno-garcia opened this issue Dec 3, 2017 · 3 comments
Open

Use ReadOnlySpan<T> instead of byte[] #9

bruno-garcia opened this issue Dec 3, 2017 · 3 comments

Comments

@bruno-garcia
Copy link
Collaborator

I'd like to propose changing the contract of ISerializer to useReadOnlySpan<T> instead of byte[].

Since System.Memory is not out yet (RTM's expected with .NET Core 2.1), obviously none of the current implementations (JSON, Xml, ProtoBuf) will actually take advantage of that. It would be great though to already use ReadOnlySpan<T> on the contract to avoid any breaking changes related to this.

When support comes from underlying libraries, we'd only change the implementations to avoid the span.ToArray() calls.

It seems to me that it doesn't make sense to expose ReadOnlySpan<T> and also byte[] so I suggest we go ahead and change the current ISerializer.

I've changed the contract to get an idea of the change: https://github.com/Greentube/serialization/tree/system.memory
Some tests break though, with:

typeof(System.InvalidProgramException): Common Language Runtime detected an invalid program.

I haven't investigated or filed a bug yet.

@bruno-garcia
Copy link
Collaborator Author

FYI @jamescrosswell @joaopgrassi

@bruno-garcia
Copy link
Collaborator Author

The reason the tests are failing is due to the lambda. That'd box Span and since the runtime disallows that, it fails.

Changing the test to this fixes the problem:

[Fact]
public void Serialize_FactoryDefined_FactoryIsCalled()
{
    var expected = new DivideByZeroException();
    var sut = new XmlSerializer(new XmlOptions
    {
        Factory = _ => throw expected
    });

    try
    {
        sut.Serialize(new object());
    }
    catch (DivideByZeroException actual)
    {
        Assert.Same(expected, actual);
    }
    catch
    {
        Assert.False(true);
    }
}

More at: http://adamsitnik.com/Span/#span-must-not-be-a-generic-type-argument

@bruno-garcia
Copy link
Collaborator Author

Simpler way to test it is just to convert the illegal Func<Span> to Action:

[Fact]
public void Serialize_FactoryDefined_FactoryIsCalled()
{
    var sut = new XmlSerializer(new XmlOptions
    {
        Factory = _ => throw new DivideByZeroException()
    });

    Assert.Throws<DivideByZeroException>(() =>
    {
        sut.Serialize(new object());
    });
}

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

No branches or pull requests

1 participant