Change System.Drawing.Common's target to netcoreapp2.0 #22833
Conversation
|
@dotnet-bot Test UWP CoreCLR x64 Debug Build |
| <RefPath>$(RefPath)</RefPath> | ||
| </BinPlaceConfiguration> | ||
| <BinPlaceConfiguration Include="$(Configuration)"> | ||
| <!-- copy shims to the testhost as well in order to be able to run the netfx tests --> |
There was a problem hiding this comment.
Why? This seems broken. TestHostRootPath is set to a BuildConfiguration-specific path, not a configuration specific path.
There was a problem hiding this comment.
Yeah, it seems wrong. It was copy-pasted from another project file (along with the $(RefPath) setting above). The shim part can probably be deleted.
| <PropertyGroup> | ||
| <BinPlaceRef>true</BinPlaceRef> | ||
| <NuGetDeploySourceItem>Reference</NuGetDeploySourceItem> | ||
| <NETCoreAppPackageVersion>2.0.0-preview2-25407-01</NETCoreAppPackageVersion> |
There was a problem hiding this comment.
Yeah, we do. I don't know why I only looked on nuget.org for this version.
Will fix.
| @@ -0,0 +1,29 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
Please add a comment to this project describing its purpose: to download a targeting pack only for a specific NETCoreApp version.
| private const short StateNameValid = 0x0008; | ||
| private const long NotDefinedValue = 0; | ||
|
|
||
| private static readonly ConstructorInfo s_ctorKnownColor; // internal Color(KnownColor knownColor) |
There was a problem hiding this comment.
I can't recall the specific differences but I know in the past we avoid caching reflection objects and instead get a delegate (either bound or unbound) so that we can have minimal overhead for the lightup after initial setup. Have a look at CreateDelegate as a potential better optimization here.
There was a problem hiding this comment.
Good idea. I believe that also lets us avoid all of the temporary array allocations associated with invoking these reflection objects.
There was a problem hiding this comment.
Actually, I'm not sure it works in this case. I am storing ConstructorInfo and FieldInfo objects, and you can't create Delegates out of those. Is that right?
There was a problem hiding this comment.
Yeah, looking back at this code we never did it for fields. For constructors we created a func to Activator.CreateInstance. FWIW, this was the code I was remembering: https://github.com/Microsoft/referencesource/blob/master/Microsoft.Bcl/System.Threading.Tasks.v1.5/System/Lightup/Lightup.cs
There was a problem hiding this comment.
Hmm, I don't really see much value in that, TBH. Is there some large benefit to calling Activator.CreateInstance(Type, object[]) rather than just invoking a cached ConstructorInfo? We don't save out on the cost of the array allocation. My intuition says the ConstructorInfo would be faster, but I can't say that I've ever tested the difference between the two 😄 .
There was a problem hiding this comment.
Yeah I don't expect CreateInstance is any faster. Perhaps is has to do with what remains rooted when you keep these objects around? Honestly I can't remember and its been years since I touched that code. It could have been due to silverlight specifics as well since that was trying to work with SL. I think what you have is fine.
|
I'm going to merge this in now so I can go ahead with my packaging work. |
|
LGTM |
| private static readonly FieldInfo s_fieldKnownColor; // private readonly short knownColor | ||
| private static readonly FieldInfo s_fieldState; // private readonly short state | ||
|
|
||
| static ColorUtil() |
There was a problem hiding this comment.
Do we think reflection is the best option here? Why not just source sharing and changing the visibility?
With dotnet#22761 and dotnet/corefx#22833, BitmapSelector.Suffix will always be null. This means this feature is dead code, and users are unable to use it. Removing this dead code because: 1. It doesn't do anything. 2. It is causing ILLink warnings, and it is easier to delete than to try to address the warnings. I logged https://github.com/dotnet/runtime/issues/54363 to follow up and either re-implement this feature, or obsolete the attributes that no longer have any effect on the app.
With #22761 and dotnet/corefx#22833, BitmapSelector.Suffix will always be null. This means this feature is dead code, and users are unable to use it. Removing this dead code because: 1. It doesn't do anything. 2. It is causing ILLink warnings, and it is easier to delete than to try to address the warnings. I logged https://github.com/dotnet/runtime/issues/54363 to follow up and either re-implement this feature, or obsolete the attributes that no longer have any effect on the app.
…etcoreapp20 Change System.Drawing.Common's target to netcoreapp2.0 Commit migrated from dotnet/corefx@87b5a4e
With dotnet/runtime#22761 and dotnet/corefx#22833, BitmapSelector.Suffix will always be null. This means this feature is dead code, and users are unable to use it. Removing this dead code because: 1. It doesn't do anything. 2. It is causing ILLink warnings, and it is easier to delete than to try to address the warnings. I logged https://github.com/dotnet/runtime/issues/54363 to follow up and either re-implement this feature, or obsolete the attributes that no longer have any effect on the app. Commit migrated from dotnet/runtime@9199eb6
There are two parts to this PR:
@ericstj @weshaggard Could you take a look at the build configuration stuff that I have changed?