-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Seal internal and private classes in corelib #49958
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
Conversation
src/coreclr/System.Private.CoreLib/Tools/GenUnicodeProp/TableLevels.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Diagnostics/StackFrameHelper.cs
Outdated
Show resolved
Hide resolved
e459790
to
94433fb
Compare
Sealing __Canon broke one ConcurrentBag test that tried to CopyTo an Exception[] into an InvalidOperationException[]... the test expected a cast exception, but in release, it didn't get one. |
// SymWrapperCore is never instantiated and is used as an encapsulation class. | ||
// It is our "ISymWrapper.dll" assembly within an assembly. | ||
//------------------------------------------------------------------------------ | ||
internal class SymWrapperCore | ||
internal sealed class SymWrapperCore |
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.
Should SymWrapperCore be made a static class?
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.
Seems like it probably can be.
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.
I think there is CA1052 rule for that ;-)
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.
I postponed turning that on in the past due to the number of changes required, but I'll take another look at it.
Contributes to #49944