Skip to content
This repository was archived by the owner on Sep 13, 2022. It is now read-only.

Miscellaneous improvements in System#823

Merged
terrajobst merged 1 commit intodotnet:masterfrom
terrajobst:system
Oct 22, 2018
Merged

Miscellaneous improvements in System#823
terrajobst merged 1 commit intodotnet:masterfrom
terrajobst:system

Conversation

@terrajobst
Copy link
Copy Markdown

@terrajobst terrajobst commented Jul 16, 2018 •
edited
Loading

This also fixes #771 and fixed #539.

@dotnet/nsboard

@terrajobst terrajobst added the netstandard-api This tracks requests for standardizing APIs. label Jul 16, 2018
@terrajobst terrajobst added this to the .NET Standard vNext milestone Jul 16, 2018
@kumpera
Copy link
Copy Markdown

kumpera commented Jul 16, 2018

This is removing ISerializable from a bunch of exception types, what's the rationale for that?

@terrajobst
Copy link
Copy Markdown
Author

terrajobst commented Jul 17, 2018 •
edited
Loading

This is removing ISerializable from a bunch of exception types, what's the rationale for that?

It was never intended to be on the derived types as the base type is implementing it.

joshpeterson
joshpeterson reviewed Jul 18, 2018
View reviewed changes
Comment thread netstandard/ref/System.cs Outdated
public static bool TryGetSwitch(string switchName, out bool isEnabled) { isEnabled = default(bool); throw null; }
}
public sealed partial class AppDomain : System.MarshalByRefObject
public partial class AppDomain : System.MarshalByRefObject
Copy link
Copy Markdown

@joshpeterson joshpeterson Jul 18, 2018

Choose a reason for hiding this comment

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

What are the use cases for deriving from AppDomain? I don't see any problems with this from the Unity side, I'm just curious.

Copy link
Copy Markdown
Author

@terrajobst terrajobst Jul 18, 2018

Choose a reason for hiding this comment

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

Not sure. @weshaggard might know.

Copy link
Copy Markdown
Member

@jkotas jkotas Jul 18, 2018

Choose a reason for hiding this comment

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

This is a bug in .NET Core. AppDomain should have stayed sealed.

Copy link
Copy Markdown
Member

@weshaggard weshaggard Jul 26, 2018

Choose a reason for hiding this comment

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

Agreed I filed https://github.com/dotnet/corefx/issues/31391.

Copy link
Copy Markdown
Author

@terrajobst terrajobst Jul 27, 2018

Choose a reason for hiding this comment

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

I've fixed it here too

tannergooding
tannergooding reviewed Jul 18, 2018
View reviewed changes
Comment thread netstandard/ref/System.cs
marek-safar
marek-safar previously requested changes Jul 25, 2018
View reviewed changes
Comment thread netstandard/ref/System.cs Outdated
Comment thread netstandard/ref/System.cs
Comment thread netstandard/ref/System.cs Outdated
Comment thread netstandard/ref/System.cs
TobiasSekan
TobiasSekan suggested changes Jul 27, 2018
View reviewed changes
Comment thread netstandard/ref/System.cs
@svick svick mentioned this pull request Aug 22, 2018
Merged
@terrajobst terrajobst requested review from a team October 15, 2018 17:13
@terrajobst
Copy link
Copy Markdown
Author

terrajobst commented Oct 15, 2018

All feedback should be resolved. Are we ready?

@terrajobst terrajobst changed the base branch from master-with-span to system-buffers October 16, 2018 02:53
@terrajobst terrajobst added the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Oct 16, 2018
@terrajobst
Copy link
Copy Markdown
Author

terrajobst commented Oct 16, 2018

@joshpeterson this is the only one you haven't signed off on. Oversight? 😄 Thanks!

@joshpeterson
Copy link
Copy Markdown

joshpeterson commented Oct 16, 2018

@terrajobst: Yes, I just missed it. Thanks!

jaredpar
jaredpar reviewed Oct 18, 2018
View reviewed changes
Comment thread netstandard/ref/System.cs
@terrajobst terrajobst requested review from a team as code owners October 22, 2018 16:51
@terrajobst terrajobst requested a review from a team as a code owner October 22, 2018 16:51
@terrajobst terrajobst removed request for a team October 22, 2018 16:52
jkotas
jkotas reviewed Oct 22, 2018
View reviewed changes
Comment thread netstandard/ref/System.cs Outdated
jkotas
jkotas reviewed Oct 22, 2018
View reviewed changes
Comment thread netstandard/ref/System.cs Outdated
@terrajobst terrajobst force-pushed the system branch 2 times, most recently from 7c39eaa to 5b6e563 Compare October 22, 2018 18:08
@terrajobst terrajobst changed the base branch from system-buffers to master October 22, 2018 18:09
@terrajobst terrajobst removed the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Oct 22, 2018
@terrajobst terrajobst merged commit 214ec3f into dotnet:master Oct 22, 2018
@terrajobst terrajobst deleted the system branch October 22, 2018 18:58
@bartonjs bartonjs mentioned this pull request Mar 3, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

9 more reviewers

@svick svick svick left review comments

@tannergooding tannergooding tannergooding left review comments

@TobiasSekan TobiasSekan TobiasSekan requested changes

@joshpeterson joshpeterson joshpeterson approved these changes

@jaredpar jaredpar jaredpar approved these changes

@marek-safar marek-safar marek-safar approved these changes

@clairernovotny clairernovotny clairernovotny approved these changes

@jkotas jkotas jkotas approved these changes

@weshaggard weshaggard weshaggard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

netstandard-api This tracks requests for standardizing APIs.

Projects

None yet

Milestone

.NET Standard 2.1

Development

Successfully merging this pull request may close these issues.

[Proposal] Add GC.GetAllocatedBytesForCurrentThread in .Net Standard vNext Missing System.String.Replace definitions in .NetStandard 2.0