-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Unnecessary cryptographic derived types obsoletions #52303
Changes from 20 commits
3d01d78
ae7b109
232775b
f7005f8
bc94bc7
adddf44
56775ea
4fdff53
89ecc58
76ce957
acf6002
7724dc9
1294d03
eef5db8
4fea03a
875a7ce
9bbaddd
f160484
ef88b0c
06f3e78
7afc9d5
665f37d
b364451
403413a
4e5bef6
f8067e9
4b0f491
5a97ee0
f13b2b7
87225b9
0d5a153
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||
<Project Sdk="Microsoft.NET.Sdk"> | ||||
<PropertyGroup> | ||||
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks> | ||||
<NoWarn>$(NoWarn);SYSLIB0021</NoWarn> | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't think of a reason why System.Runtime.Serialization.Xml.Tests would need the NoWarn. If it's explicitly testing serialization for one of the obsoleted types, a pragma around that/those test(s) is more appropriate. Otherwise, it would probably be a place that's doing the old/wrong pattern that should be updated to better code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem was here. static MD5CryptoServiceProvider md5 = null; // <--- line 211 and if (md5 == null)
md5 = new MD5CryptoServiceProvider(); // <--- line 270 The method using this type: runtime/src/libraries/System.Runtime.Serialization.Xml/tests/SerializationTestTypes/DataContract.cs Line 267 in 1b30c00
What then needs to be done? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, this code is just an example of the bad pattern. So we should fix it 😄
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, thanks for the instructions 🙂 Just fixed! |
||||
</PropertyGroup> | ||||
<ItemGroup> | ||||
<Compile Include="$(TestSourceFolder)DataContractSerializerStressTests.cs" /> | ||||
|
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.
When possible, the message should also indicate what should be done to resolve the warning. Is the following accurate, @bartonjs?
When this is changed, it will need to be updated in
Obsoletions.cs
and theref
sources too.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.
It's technically correct. I don't know if "underlying type" is the best wording, but I can't come up with an obviously better one.
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.
@gewarren do you have a suggestion for a message better than this?
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'd suggest "...Use the Create method on the base type instead."
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.
@jeffhandley apply this?
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.
Yes please. Thanks!
Side note -- I'm sorry about the trouble with the project file; I hope you didn't mind that I pushed commits to fix it. Since that was an existing issue unrelated to your changes, I didn't want to burden you with fixing that up. I'm glad this PR uncovered it though!
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.
It's okay, of course I was not against helping with the resolution of problems in the project file! Finished with the documentation :)