Skip to content

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Apr 28, 2022

This is related to #68189. It seems like we hand-edit the ref assembly files and/or the tool generating them changed. Ideally, this is really no-op with exception of SslClientHelloInfo that was missing the magic _dummy fields.

This was generated with msbuild /t:GenerateReferenceAssemblySource
With this, adding new API and generating reference should just show the new change instead of this big diff.

Note that I also tried to do it for HTTP but it generated ref that broke the build. I'll try to investigate separately.

@@ -699,10 +686,10 @@ public partial class MediaTypeHeaderValue : System.ICloneable
         public override int GetHashCode() { throw null; }
         public static System.Net.Http.Headers.MediaTypeHeaderValue Parse(string? input) { throw null; }
-        object System.ICloneable.Clone() { throw null; }
+        object? System.ICloneable.Clone() { throw null; }
         public override string ToString() { throw null; }

@wfurt wfurt requested review from a team, ericstj, jkotas and stephentoub April 28, 2022 00:49
@wfurt wfurt self-assigned this Apr 28, 2022
@ghost
Copy link

ghost commented Apr 28, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This is related to #68189. It seems like we hand-edit the ref assembly files and/or the tool generating them changed. Ideally, this is really no-op with exception of SslClientHelloInfo that was missing the magic _dummy fields.

This was generated with msbuild /t:GenerateReferenceAssemblySource
With this, adding new API and generating reference should just show the new change instead of this big diff.

Note that I also tried to do it for HTTP but it generated ref that broke the build. I'll try to investigate separately.

@@ -699,10 +686,10 @@ public partial class MediaTypeHeaderValue : System.ICloneable
         public override int GetHashCode() { throw null; }
         public static System.Net.Http.Headers.MediaTypeHeaderValue Parse(string? input) { throw null; }
-        object System.ICloneable.Clone() { throw null; }
+        object? System.ICloneable.Clone() { throw null; }
         public override string ToString() { throw null; }
Author: wfurt
Assignees: wfurt
Labels:

area-System.Net

Milestone: -

@ghost
Copy link

ghost commented Apr 28, 2022

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

public override void Write(System.ReadOnlySpan<byte> buffer) { }
public override System.Threading.Tasks.Task WriteAsync(byte[] buffer, int offset, int count, System.Threading.CancellationToken cancellationToken) { throw null; }
public override System.Threading.Tasks.ValueTask WriteAsync(System.ReadOnlyMemory<byte> buffer, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public override System.Threading.Tasks.ValueTask WriteAsync(System.ReadOnlyMemory<byte> buffer, System.Threading.CancellationToken cancellationToken) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

dtto

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed. It seems like the overload with array does not have default while the ReadOnlyMemor does. I don't know if that is oversight or intention. Should we change it? (may be separate PR as that would be perhaps functional change.

Copy link
Member

Choose a reason for hiding this comment

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

the overload with array does not have default

It is intentional. For arrays, there are two different ReadAsync methods: One that takes CancellationToken and second one that does not take CancellationToken (inherited from base class).

public readonly partial struct SslClientHelloInfo
{
private readonly object _dummy;
private readonly int _dummyPrimitive;
Copy link
Member

Choose a reason for hiding this comment

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

Note this could cause some existing compiling code to stop compiling. (But we should still fix it.)

@wfurt wfurt merged commit 42c956b into dotnet:main May 3, 2022
@wfurt wfurt deleted the ref branch May 3, 2022 23:24
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants