Skip to content
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

Resolve remaining Xslt-related warnings (round 2) #50211

Merged
merged 7 commits into from
Mar 27, 2021

Conversation

joperezr
Copy link
Member

Addressing the remaining warnings in System.Private.Xml related to xslt. I have one more batch of changes to be pushed to this which are almost ready and will deal with one more warning left. I'll add a comment here when this is ready for review.

Contributes to #45623

@ghost
Copy link

ghost commented Mar 24, 2021

Tagging subscribers to this area: @buyaa-n, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

Addressing the remaining warnings in System.Private.Xml related to xslt. I have one more batch of changes to be pushed to this which are almost ready and will deal with one more warning left. I'll add a comment here when this is ready for review.

Contributes to #45623

Author: joperezr
Assignees: -
Labels:

area-System.Xml

Milestone: -

@joperezr joperezr added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 24, 2021
@ghost
Copy link

ghost commented Mar 24, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

Addressing the remaining warnings in System.Private.Xml related to xslt. I have one more batch of changes to be pushed to this which are almost ready and will deal with one more warning left. I'll add a comment here when this is ready for review.

Contributes to #45623

Author: joperezr
Assignees: -
Labels:

area-System.Xml, linkable-framework

Milestone: -

@joperezr
Copy link
Member Author

PR is now ready for review 😃

@@ -2817,6 +2817,7 @@ public partial class XsltArgumentList
{
public XsltArgumentList() { }
public event System.Xml.Xsl.XsltMessageEncounteredEventHandler XsltMessageEncountered { add { } remove { } }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("The stylesheet may have calls to methods of the extension object passed in which cannot be statically analyzed by the trimmer. Ensure all methods that may be called are kept.")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("The stylesheet may have calls to methods of the extension object passed in which cannot be statically analyzed by the trimmer. Ensure all methods that may be called are kept.")]
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("The stylesheet may have calls to methods of the extension object passed in which cannot be statically analyzed by the trimmer. Ensure all methods that may be called are preserved.")]

I've been using the term preserved elsewhere. It would be good to be consistent throughout the codebase.

Copy link
Member

@eerhardt eerhardt Mar 26, 2021

Choose a reason for hiding this comment

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

I don't think this was resolved in the latest code. Both here and below.

@joperezr
Copy link
Member Author

@vitek-karas @eerhardt thanks a lot for your feedback! PR is ready in case you want to take another look.

@@ -1054,6 +1054,9 @@ public XslFlags Variable(string prefix, string name)
return XslFlags.None;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "Supressing warning about not having the RequiresUnreferencedCode attribute since xsl Scripts are " +
"not supported in .NET Core")]
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is the dead code that can be removed? Can we add a link to the issue tracking removing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

#50308 done.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks @joperezr. I just had a few nits here and there.

@joperezr joperezr merged commit 738cd9a into dotnet:main Mar 27, 2021
@joperezr joperezr deleted the LinkerXml2 branch March 27, 2021 15:59
@ghost ghost locked as resolved and limited conversation to collaborators Apr 26, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Xml linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants