Added System.Memory.Polyfill Project #2339

KrzysztofCwalina · 2018-06-04T20:57:16Z

This project is a skeleton for an adapter package that allows writing portable code using Span<T>-enabled APIs that are found in .NET Core 2.1 but not in other versions/redists of .NET.

For example, .NET Core 2.1 Stream has a Read method that takes a Span<byte> parameter. This member does not exist in other .NET versions, which makes is difficult to write portable code (such portable code has to "manually" cross-target).

This library hide cross-targeting inside adapter APIs.

benaadams · 2018-06-04T21:03:12Z

src/System.Memory.Polyfill/System/Parse.cs

+{
+    public static partial class Int32Polyfill
+    {
+        public static bool TryParse(ReadOnlySpan<char> buffer, out int value)


Would,'t work for implicit cast from Span<char>?

Why not? It's not an extension method.

Ah, true... Is a bit ugly then :)

Static extension method would be better dotnet/csharplang#192 though alas not a thing...

ericstj · 2018-06-04T21:44:18Z

src/System.Memory.Polyfill/System.Memory.Polyfill.csproj

+    <Description>Adapters for Span APIs</Description>
+    <TargetFrameworks>netstandard1.3;netcoreapp2.1</TargetFrameworks>
+    <PackageTags>Span Memory Adapter</PackageTags>
+    <!-- disable automatic compile item inclusion so that we can set the correct metadata on TextTemplate-generated sources -->


You don't actually have any of these, so you can remove this and the compile entry below.

ericstj · 2018-06-04T21:45:13Z

src/System.Memory.Polyfill/System/Parse.cs

+        public static bool TryParse(ReadOnlySpan<char> buffer, out int value)
+        {
+#if NETCOREAPP2_1
+            return int.TryParse(buffer, out value);


You may want to avoid using the versioned define as it makes it more difficult if you decide to retarget. Instead use #if NETCOREAPP

But this api exists only in netcoreapp 2.1

You aren't defining targeting here, you're just ifdefing the code. Imagine that you wanted to cross-compile for netcoreapp2.2 as well in the future, you'd need to touch this ifdef for no reason. I think you're more likely to cross-compile for a future version of netcoreapp than you are an older version that doesn't have this API.
Alternatively you can define your own feature flag that represents what you want and make sure that's defined as appropriate.

Yeah, I agree it would work today, but it won't if we ever add new targets. Correct?

ericstj · 2018-06-04T21:47:05Z

tests/System.Memory.Polyfill.Tests/System.Memory.Polyfill.Tests.csproj

+  <Import Project="..\..\tools\common.props" />
+  <PropertyGroup>
+    <TargetFrameworks>net46;netcoreapp2.1</TargetFrameworks>
+    <AssemblyOriginatorKeyFile>../../tools/test_key.snk</AssemblyOriginatorKeyFile>


All the properties from here on look unnecessary for a test project.

yeah, I just copied them from some other test, but I will remove/simplify. Thanks.

ahsonkhan · 2018-06-05T01:01:29Z

src/System.Memory.Polyfill/System.Memory.Polyfill.csproj

+  </PropertyGroup>
+  <ItemGroup>
+    <PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" />
+    <PackageReference Include="System.Buffers" Version="$(SystemMemoryVersion)" />


Use SystemBuffersVersion instead of SystemMemoryVersion.

ahsonkhan · 2018-06-05T01:02:09Z

src/System.Memory.Polyfill/System/Stream.cs

+
+namespace System
+{
+    public static partial class MemoryPolyfill


StreamPolyfill instead?

I think we want to minimize the number of extensions types.

ahsonkhan · 2018-06-05T01:06:31Z

tests/System.Memory.Polyfill.Tests/Int32.cs

+        public void Int32TryParse()
+        {
+            char[] buffer = int.MaxValue.ToString().ToCharArray();
+            ReadOnlySpan<char> span = buffer.AsSpan();


ReadOnlySpan<char> span = int.MaxValue.ToString().AsSpan();?

KrzysztofCwalina · 2018-06-05T15:44:06Z

@dotnet-bot test Innerloop Ubuntu16.04 Release Build and Test

ahsonkhan · 2018-06-05T19:18:45Z

tests/System.Memory.Polyfill.Tests/System.Memory.Polyfill.Tests.csproj

    <TargetFrameworks>net46;netcoreapp2.1</TargetFrameworks>
-    <AssemblyOriginatorKeyFile>../../tools/test_key.snk</AssemblyOriginatorKeyFile>


Why did you remove these properties?

@ericstj suggested that they were not necessary for tests.

ahsonkhan · 2018-06-05T19:19:33Z

tests/System.Memory.Polyfill.Tests/System.Memory.Polyfill.Tests.csproj

@@ -1,13 +1,10 @@
 <Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="..\..\tools\common.props" />
  <PropertyGroup>
+    <TargetFrameworks>netcoreapp2.1</TargetFrameworks>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(TargetsWindows)' == 'true'">


Is listing netcoreapp2.1 TFM here redundant or required (for Windows only)?

I changed it in the new PR. This actually did not work in the CI. I changed it to use netcoreapp2.0 to force the fallback path.

ahsonkhan · 2018-06-05T19:20:10Z

tests/System.Memory.Polyfill.Tests/Int32.cs

@@ -11,8 +11,7 @@ public class Int32Tests
        [Fact]
        public void Int32TryParse()
        {
-            char[] buffer = int.MaxValue.ToString().ToCharArray();
-            ReadOnlySpan<char> span = buffer.AsSpan();
+            ReadOnlySpan<char> span = int.MaxValue.ToString().ToCharArray().AsSpan();


Why is the .ToCharArray() necessary?

fixed in the new PR

KrzysztofCwalina added 12 commits March 20, 2018 08:57

Bug fix in BufferWriter

c8d31c4

Merge branch 'master' of https://github.com/dotnet/corefxlab

8ee6979

Merge branch 'master' of https://github.com/dotnet/corefxlab

33da66f

Merge branch 'master' of https://github.com/dotnet/corefxlab

ee23d6b

Merge branch 'master' of https://github.com/dotnet/corefxlab

132efef

Merge branch 'master' of https://github.com/dotnet/corefxlab

f6d44ca

Merge branch 'master' of https://github.com/dotnet/corefxlab

954f079

Merge branch 'master' of https://github.com/dotnet/corefxlab

3c3b978

Merge branch 'master' of https://github.com/dotnet/corefxlab

6b160a7

Merge branch 'master' of https://github.com/dotnet/corefxlab

a5dfae9

Merge branch 'master' of https://github.com/dotnet/corefxlab

b89ba55

Merge branch 'master' of https://github.com/dotnet/corefxlab

23415be

KrzysztofCwalina requested review from terrajobst, stephentoub, joshfree, davidfowl and ericstj June 4, 2018 20:58

Added System.Memory.Polyfill Project

8a5e662

KrzysztofCwalina force-pushed the Polyfill branch from df9587e to 8a5e662 Compare June 4, 2018 21:01

benaadams reviewed Jun 4, 2018

View reviewed changes

ericstj reviewed Jun 4, 2018

View reviewed changes

ahsonkhan reviewed Jun 5, 2018

View reviewed changes

KrzysztofCwalina force-pushed the Polyfill branch from 8d4054a to 0b2a129 Compare June 5, 2018 15:37

PR review feedback and build fix

42e41c5

KrzysztofCwalina force-pushed the Polyfill branch from 0b2a129 to 42e41c5 Compare June 5, 2018 15:52

KrzysztofCwalina merged commit 697d518 into dotnet:master Jun 5, 2018

ahsonkhan reviewed Jun 5, 2018

View reviewed changes

KrzysztofCwalina deleted the Polyfill branch October 26, 2018 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added System.Memory.Polyfill Project #2339

Added System.Memory.Polyfill Project #2339

KrzysztofCwalina commented Jun 4, 2018

benaadams Jun 4, 2018

stephentoub Jun 4, 2018

benaadams Jun 4, 2018

ericstj Jun 4, 2018

KrzysztofCwalina Jun 4, 2018

ericstj Jun 4, 2018

KrzysztofCwalina Jun 4, 2018

ericstj Jun 4, 2018

KrzysztofCwalina Jun 5, 2018

ericstj Jun 4, 2018

KrzysztofCwalina Jun 4, 2018

ahsonkhan Jun 5, 2018

ahsonkhan Jun 5, 2018 •

edited

Loading

KrzysztofCwalina Jun 5, 2018

ahsonkhan Jun 5, 2018

KrzysztofCwalina commented Jun 5, 2018

ahsonkhan Jun 5, 2018

KrzysztofCwalina Jun 5, 2018

ahsonkhan Jun 5, 2018

KrzysztofCwalina Jun 5, 2018

ahsonkhan Jun 5, 2018

KrzysztofCwalina Jun 5, 2018

		<TargetFrameworks>net46;netcoreapp2.1</TargetFrameworks>
		<AssemblyOriginatorKeyFile>../../tools/test_key.snk</AssemblyOriginatorKeyFile>

Added System.Memory.Polyfill Project #2339

Added System.Memory.Polyfill Project #2339

Conversation

KrzysztofCwalina commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jun 5, 2018 •

edited

Loading