-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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
C#: add System.Memory dependency #5835
C#: add System.Memory dependency #5835
Conversation
@jskeet WDYT about changing the target from |
Looks like this change is mutually exclusive with #5445. I'll reach out to the internal user that requested downgrading LangVersion to see what we can do. |
I think I'd prefer to add a netstandard2.0 target (which comes with a bunch of benefits for 2.0+ users) and only conditionally include System.Memory things. (Jan, I know a bit more about the LangVersion change - ping me if you need to.) |
@@ -7,7 +7,7 @@ | |||
<VersionPrefix>3.7.0</VersionPrefix> | |||
<LangVersion>6</LangVersion> | |||
<Authors>Google Inc.</Authors> | |||
<TargetFrameworks>netstandard1.0;net45</TargetFrameworks> | |||
<TargetFrameworks>netstandard1.1;net45</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be considered a breaking change no? Adding another target for netstandard2.0 (or netstandard 1.1 if you want) would probably be better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="SourceLink.Create.CommandLine" PrivateAssets="All" Version="2.7.6"/> | ||
<PackageReference Include="SourceLink.Create.CommandLine" PrivateAssets="All" Version="2.7.6" /> | ||
<PackageReference Include="System.Memory" Version="4.5.2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new dependency is also usually considered a "breaking change". Maybe make this conditional on the target framework?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new dependency is also usually considered a "breaking change". Maybe make this conditional on the target framework?
Adding a new dependency isn't a breaking change. Removing one is though. I think we should potentially bump the minor version here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor version will be bumped because this change won't be available before protobuf v3.8.x is out.
I'll wait for #5838 to be merged first. |
e37379c
to
3333fc2
Compare
I added |
FYI I'm halfway through a large PR to add span-based reader and writer for protobuf. Should be able to create a PR in the next day or two. |
also add useful Span-based methods for ByteString
3333fc2
to
0c85e16
Compare
@anandolee @jskeet can we review and merge? |
I think in the |
Also add useful Span-based methods for ByteString
System.Memory dependency is only added for netstandard2.0 target, netstandard1.0 and net45 stay unchanged (we actually could add System.Memory dependency for net45 too, but it seems that most users that will benefit from Span optimizations will already have access to netstandard2.0).
This PR is mostly preparation for future span-based optimizations, but to make at least some use of the new dependency, two utility ByteString methods are being added.
See grpc/grpc-dotnet#30 for more details about the planned serialization/deserialization API.