Visitar URL original
XmlHelper - Check char.IsHighSurrogate before checking length by snakefoot · Pull Request #6273 · NLog/NLog · GitHub
Skip to content

XmlHelper - Check char.IsHighSurrogate before checking length - #6273

Merged
snakefoot merged 1 commit into
NLog:devfrom
snakefoot:XmlHelperSurrogate
Sep 5, 2026
Merged

XmlHelper - Check char.IsHighSurrogate before checking length#6273
snakefoot merged 1 commit into
NLog:devfrom
snakefoot:XmlHelperSurrogate

Conversation

@snakefoot

@snakefoot snakefoot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: ae7c4f63-4819-4670-8026-796f6b6ae858

📥 Commits

Reviewing files that changed from the base of the PR and between 67f2735 and 3c96c23.

📒 Files selected for processing (1)
  • src/NLog/Internal/XmlHelper.cs

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Walkthrough

XmlHelper now uses char.IsSurrogate and inline surrogate-pair checks. EscapeCData also changes a local declaration from const string to var.

Changes

XML surrogate handling

Layer / File(s) Summary
Inline surrogate checks
src/NLog/Internal/XmlHelper.cs		 ExoticIsXmlChar, RemoveInvalidXmlChars, CreateValidXmlString, and EscapeXmlString use direct surrogate checks. EscapeCData uses var for emptyCData.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 3c96c

XML surrogate-pair handling is simplified without changing the observed validation behavior, and no current merge-blocking risk remains.

Poem

A rabbit checks each XML char,
With ears alert and eyes ajar.
High pairs hop with lows in tow,
Invalid marks are told to go.
Inline checks now guide the way,
While var helps CData stay.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No pull request description was provided, so the change rationale and scope are not documented. Add a concise description that summarizes the XmlHelper surrogate-check refactoring and related changes.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the primary surrogate-checking change in XmlHelper and is concise.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

sonarqubecloud Bot commented Sep 5, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@snakefoot
snakefoot merged commit a084ce2 into NLog:dev Sep 5, 2026
6 checks passed
@snakefoot snakefoot added this to the 6.2.1 milestone Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

refactoring size/S

Projects

None yet

Milestone

6.2.1

Development

Successfully merging this pull request may close these issues.

1 participant

@snakefoot