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

Nullable: System.Xml, part 6 (XSLT minus XSLT.Runtime) #40368

Merged
merged 5 commits into from
Aug 15, 2020

Conversation

krwq
Copy link
Member

@krwq krwq commented Aug 5, 2020

XSLT minus:

  • *XPath* files which @jozkee currently works on
  • XSLT.Runtime

@krwq krwq requested a review from a team August 5, 2020 07:43
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Aug 5, 2020

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

Comment on lines +121 to +125
string uriString = (string)info.GetValue("Uri", typeof(string))!;
int startLine = (int)info.GetValue("StartLine", typeof(int))!;
int startPos = (int)info.GetValue("StartPos", typeof(int))!;
int endLine = (int)info.GetValue("EndLine", typeof(int))!;
int endPos = (int)info.GetValue("EndPos", typeof(int))!;
Copy link
Member

Choose a reason for hiding this comment

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

What ensures that info.GetValue will never be null?

Copy link
Member

Choose a reason for hiding this comment

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

This .ctor is only referenced by another internal .ctor in XPathCompileException which shows zero references so this might as well be dead code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think those might be used by BinaryFormatter. I'd refrain from touching this. While technically payload can be tampered, in normal case you will get only what you wrote which should not affect annotation.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Left some questions on public API annotations.

{
if (input == null)
{
throw new ArgumentNullException(nameof(input));
}
return Transform(input.CreateNavigator(), args, resolver);
return Transform(input.CreateNavigator()!, args, resolver);
Copy link
Member

Choose a reason for hiding this comment

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

These node types can return null:

case XmlNodeType.EntityReference:
case XmlNodeType.Entity:
case XmlNodeType.DocumentType:
case XmlNodeType.Notation:
case XmlNodeType.XmlDeclaration:
return null;

Could that cause an issue later on this code?

Copy link
Member Author

Choose a reason for hiding this comment

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

theoretically yes but it's not expected to pass any of these here so it should produce the error - perhaps error could be better... Not sure how would transformation work on say EntityReference... it only make sense for document or element and perhaps some kind of text node (even that would be a stretch)

Copy link
Member Author

Choose a reason for hiding this comment

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

note this class is Obsolete anyway so I wouldn't even bother filing issue on this

@krwq
Copy link
Member Author

krwq commented Aug 7, 2020

hmm, I'm not getting these build errors locally, will try rebasing

@krwq krwq force-pushed the nullable-xml-xslt branch from 3c80c5a to 9dba56c Compare August 7, 2020 17:19
@krwq krwq merged commit 12f8e34 into dotnet:master Aug 15, 2020
@krwq
Copy link
Member Author

krwq commented Aug 15, 2020

Thanks @jozkee!

@krwq
Copy link
Member Author

krwq commented Aug 15, 2020

ref: #2339

@jeffhandley
Copy link
Member

💯 Awesome!!!

@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants