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

Add basic Span<T> support #483

Merged
merged 19 commits into from
Sep 24, 2023
Merged

Conversation

DaZombieKiller
Copy link
Contributor

This pull request adds basic support for Span<T> and ReadOnlySpan<T> to the AsmResolver and AsmResolver.IO namespaces.

Support is only implemented for .NET Standard 2.1 and higher, no attempt has been made to provide Span<T> support for .NET Standard 2.0 as it would introduce a dependency on System.Memory.

This PR also does not attempt to take advantage of this new support within the library. This is most likely better left to a separate PR (and/or set of issues).

The following new types are provided to avoid source and binary breaking changes in existing code:

namespace AsmResolver.IO
{
    public interface ISpanDataSource : IDataSource
    {
        int ReadBytes(ulong address, Span<byte> buffer);
    }

    public interface ISpanBinaryStreamWriter : IBinaryStreamWriter
    {
        void WriteBytes(ReadOnlySpan<byte> buffer);
    }

    public static class DataSourceExtensions
    {
        public static int ReadBytes(this IDataSource dataSource, ulong address, Span<byte> buffer);
    }
}

ISpanDataSource and ISpanBinaryStreamWriter can be considered temporary, as they only exist to avoid the breaking change of adding a new member to the corresponding IDataSource and IBinaryStreamWriter interfaces. In the future, it may be desirable to remove them and take the break.

@ds5678
Copy link
Contributor

ds5678 commented Sep 18, 2023

It might be a good idea to add a define variable, eg SPAN_SUPPORTED, instead of using NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER for the preprocessor directives.

@Washi1337 Washi1337 added this to the 5.5.0 milestone Sep 18, 2023
@DaZombieKiller
Copy link
Contributor Author

@ds5678

It might be a good idea to add a define variable, eg SPAN_SUPPORTED, instead of using NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER for the preprocessor directives.

I thought about doing this, but decided to wait for feedback to see what would be preferred. Since there aren't any plans to expose this under .NET Standard 2.0, SPAN_SUPPORTED will never mean anything other than NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER, but I suppose it is much less noisy in the source.

@Washi1337
Copy link
Owner

I have no real strong opinions on introducing either SPAN_SUPPORTED or leaving it as an OR expression as it is now. If it is easy to add SPAN_SUPPORTED without too much messing around in the csprojs, then I'm all for it. If not, we can leave it as is as well.

@Washi1337
Copy link
Owner

I just realized your PR is based on master. Can you please change the base branch to development and see if all the tests still succeed?

@DaZombieKiller DaZombieKiller changed the base branch from master to development September 24, 2023 15:25
@Washi1337 Washi1337 merged commit 46e56cb into Washi1337:development Sep 24, 2023
1 check passed
@Washi1337
Copy link
Owner

Thanks!

@DaZombieKiller DaZombieKiller deleted the span-support branch September 24, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants