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 api diff for preview5 #6330

Merged
merged 2 commits into from
Jun 8, 2021
Merged

add api diff for preview5 #6330

merged 2 commits into from
Jun 8, 2021

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Jun 4, 2021

``` diff
namespace Microsoft.Win32.SafeHandles {
+ public abstract class SafeNCryptHandle : SafeHandleZeroOrMinusOneIsInvalid {
+ protected SafeNCryptHandle();
Copy link
Member

Choose a reason for hiding this comment

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

What's new here? Most of this stuff previously existed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we added openssl and cng dlls to the ref pack cc @ericstj @ViktorHofer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The implementation was in the shared framework already, but the reference assembly was OOB because of single-platform support. Now that we have the platform analyzer stuff we got rid of the package, so this is probably showing up as being "new in the shared framework reference API set"

Copy link
Member

Choose a reason for hiding this comment

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

If we have a reasonable way of leaving all of the SafeNCrypt*Handle types out, I think that'd be best. It's not "new API" so much as "we changed some layout".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i can go ahead and remove them.

``` diff
namespace System.Numerics {
- public struct Complex : IEquatable<Complex>, IFormattable
+ public readonly struct Complex : IEquatable<Complex>, IFormattable
Copy link
Member

Choose a reason for hiding this comment

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

Changes to Complex and Vector LGTM.

CC. @pgovind

``` diff
namespace System.Runtime.CompilerServices {
public static class RuntimeFeature {
+ public const string VirtualStaticsInInterfaces = "VirtualStaticsInInterfaces";
Copy link
Member

Choose a reason for hiding this comment

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

CC. @jeffhandley that this made Preview 5 for the generic-math work.

Copy link

Choose a reason for hiding this comment

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

Ah awesome, I didn't realize this went it already!


``` diff
namespace System.Runtime.Versioning {
+ public sealed class RequiresPreviewFeaturesAttribute : Attribute {
Copy link
Member

Choose a reason for hiding this comment

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

RequiresPreviewFeaturesAttribute LGTM.

CC. @pgovind

@tarekgh
Copy link
Member

tarekgh commented Jun 4, 2021

System.Diagnostics looks good to me.

+ public void Encrypt(ReadOnlySpan<byte> nonce, ReadOnlySpan<byte> plaintext, Span<byte> ciphertext, Span<byte> tag, ReadOnlySpan<byte> associatedData = default(ReadOnlySpan<byte>));
+ }
+ public sealed class CngAlgorithm : IEquatable<CngAlgorithm> {
+ public CngAlgorithm(string algorithm);
Copy link
Member

Choose a reason for hiding this comment

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

Same question here... hasn't a lot of this stuff previously existed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we added openssl and cng dlls to the ref pack cc @ericstj @ViktorHofer

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, System.Security.Cryptography wouldn't show any of the Cng or OpenSsl types as a diff (as with the SafeHandles).

I'd expect to just see

  • AesCcm.IsSupported
  • AesGcm.IsSupported
  • ChaCha20Poly1305 (the type and all its members)

``` diff
namespace System {
public abstract class Array : ICloneable, ICollection, IEnumerable, IList, IStructuralComparable, IStructuralEquatable {
+ public static int MaxLength { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Array.MaxLength addition LGTM. CC. @pgovind

# System.Text.Json.Node

``` diff
-namespace System.Text.Json.Node {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible/better for this to show up as a change in namespace name rather than massive deletion / addition of everything in the namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can think of a trivial way to automatically detect this. But i can create an issue in arcade to see if we can do this.
https://github.com/dotnet/arcade/tree/main/src/Microsoft.DotNet.AsmDiff most of the core code base is similar to this (in case u r interested)


``` diff
namespace Microsoft.AspNetCore.Authentication.Cookies {
public interface ITicketStore {
Copy link
Member

Choose a reason for hiding this comment

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

# Microsoft.AspNetCore.Connections

``` diff
namespace Microsoft.AspNetCore.Connections {
Copy link
Member

Choose a reason for hiding this comment

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

``` diff
namespace Microsoft.AspNetCore.Http.Connections {
public class HttpConnectionDispatcherOptions {
+ public TimeSpan TransportSendTimeout { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

# Microsoft.AspNetCore.Http

``` diff
namespace Microsoft.AspNetCore.Http {
Copy link
Member

Choose a reason for hiding this comment

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

@shirhatti This needs a good call out with examples in the blog post!

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

ASP.NET looks good.


``` diff
namespace Microsoft.Extensions.DependencyInjection {
+ public readonly struct AsyncServiceScope : IAsyncDisposable, IDisposable, IServiceScope {
Copy link
Member

Choose a reason for hiding this comment

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

👍🏾

Copy link

@pgovind pgovind left a comment

Choose a reason for hiding this comment

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

LGTM for Numerics and Runtime.Versioning

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

System.IO :shipit:

@Anipik
Copy link
Contributor Author

Anipik commented Jun 7, 2021

@bartonjs I removed the extra stuff, can you take a look ?

@Anipik Anipik merged commit 2f31567 into dotnet:main Jun 8, 2021
@Anipik Anipik deleted the preview5 branch June 8, 2021 16:39
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

Successfully merging this pull request may close these issues.