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.

Sign up for GitHub

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

Jump to bottom

Use char override Writer for System.Private.Xml #61722

Closed
wants to merge 3 commits into from
Closed

Use char override Writer for System.Private.Xml #61722

wants to merge 3 commits into from

Conversation

kronic
Copy link
Contributor

@kronic kronic commented Nov 17, 2021

No description provided.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 17, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Xml and removed community-contribution Indicates that the PR has been added by a community member labels Nov 17, 2021
@ghost
Copy link

ghost commented Nov 17, 2021

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: kronic
Assignees: -
Labels:

area-System.Xml
Milestone: -

eiriktsarpalis
eiriktsarpalis reviewed Nov 17, 2021
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

I welcome making the code more consistent --even though any performance benefits are likely theoretical-- but does this actually change all occurrences of single-character strings in the codebase? A simple regex search seems to indicate that there many more that this PR is not touching. Note that this does not limit to TextWriter.Write overloads, there also seem to be other internal classes with a similar API pattern, such as IndentedWriter.

@kronic
Copy link
Contributor Author

kronic commented Nov 18, 2021

@eiriktsarpalis done

@eiriktsarpalis eiriktsarpalis added the community-contribution Indicates that the PR has been added by a community member label Nov 18, 2021
@jeffhandley
Copy link
Member

I've marked this PR as "needs more info" for now. We'd like to better understand the risk/reward for the series of XML changes. See #61773 (comment). Thanks!

drieseng
drieseng reviewed Nov 19, 2021
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs
@@ -274,7 +274,7 @@ internal virtual void WriteQualifiedName(string prefix, string localName, string
if (prefix.Length != 0)
{
WriteString(prefix);
WriteString(":");
WriteCharEntity(':');
Copy link
Contributor

@drieseng drieseng Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why would you want to create a character entity here? (and in the other places that you modified)

@eiriktsarpalis eiriktsarpalis added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs more info labels Nov 30, 2021
@ghost ghost added the no-recent-activity label Dec 14, 2021
@ghost
Copy link

ghost commented Dec 14, 2021

This pull request has been automatically marked no recent activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no recent activity.

@kronic kronic closed this Dec 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kasperk81 kasperk81 kasperk81 left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis left review comments

@drieseng drieseng drieseng left review comments

Assignees

@krwq krwq

Labels
area-System.Xml community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kronic @jeffhandley @drieseng @eiriktsarpalis @kasperk81 @krwq