Introduce Component as the base type for a few components#12324
Introduce Component as the base type for a few components#12324AlexGhiondea merged 2 commits intodotnet:masterfrom
Conversation
| namespace System.ComponentModel | ||
| { | ||
| public class BackgroundWorker : IDisposable | ||
| public class BackgroundWorker : System.ComponentModel.Component |
There was a problem hiding this comment.
IIRC, because Component wasn't a base class, we added finalizers to some of these derived types like BackgroundWorker, so that if types derived from BackgroundWorker and override Dispose(bool), they would still have their Dispose method called on finalization (since Component has one). Now that we're adding back Component, we should remove such finalizers we added just to core. In addition to being unnecessary, they could actually lead to problems, if they result in Dispose(false) being executed twice for an instance.
There was a problem hiding this comment.
Indeed -- there were quite a few places where I had to remove the Dispose method we introduced before this.
| /// information about the module. | ||
| /// </devdoc> | ||
| public class ProcessModule | ||
| public class ProcessModule : System.ComponentModel.Component |
There was a problem hiding this comment.
For all of these, can we add using System.ComponentModel; rather than including the namespace here?
|
A few comments, otherwise LGTM |
The following types were changed to derive from Component: - WebClient - FileSystemWatcher - Ping - BackgroundWorker - Process - ProcessModule - ProcessThread
AlexGhiondea
force-pushed
the
ComponentAsBaseType_12154
branch
from
7f0ce62 to
94f1481
Compare
| </ItemGroup> | ||
| <ItemGroup> | ||
| <ItemGroup Condition="'$(TargetGroup)'==''"> | ||
| <ProjectReference Include="..\..\pkg\System.Net.Ping.pkgproj"> |
There was a problem hiding this comment.
This pkgproj ProjectReference should work for both 1.3 and 1.7 configurations. Why did you need to move to a package reference for some cases.
There was a problem hiding this comment.
Without this the netcoreapp1.0 tests were not able to resolve the asset for it.
| <OSGroup>Windows_NT</OSGroup> | ||
| <TargetGroup>netstandard1.3</TargetGroup> | ||
| </Project> | ||
| <Project Include="FunctionalTests\System.Net.Ping.Functional.Tests.csproj"> |
There was a problem hiding this comment.
Please put the default TargetGroup in the group at the top of the file see #12312 for reference.
|
@AlexGhiondea, you didn't remove the finalizers as I mentioned? |
|
@stephentoub I was 100% sure you meant the Dispose methods we added. Created #12360 to remove the actual finalizers. |
…aseType_12154 Introduce Component as the base type for a few components Commit migrated from dotnet/corefx@f1b7585
5 participants
The following types were changed to derive from Component:
Fixes #12154