Skip to content

Conversation

@snakefoot
Copy link
Contributor

No description provided.

@snakefoot snakefoot added this to the 6.0.6 milestone Nov 8, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 8, 2025

Walkthrough

Reworks internal XML parsing in XmlParser.cs: localizes current-char handling, tightens Unicode/hex/token/name validation, standardizes error messages with line info, streamlines attributes/CDATA/processing-instruction parsing, and replaces some index-based loops with foreach — no public API changes.

Changes

Cohort / File(s) Change Summary
Parser core & flow
src/NLog/Internal/XmlParser.cs
Localizes _xmlSource.Current into local chr variables, replaces some index-based loops with foreach, simplifies stack/root local usage, and removes several intermediate temporaries.
Name/token/char validation
src/NLog/Internal/XmlParser.cs
Consolidates character checks using explicit range/IsLetterOrDigit logic, validates each token/end-tag char in per-character loops, and tightens invalid-character error messages.
Unicode / hex parsing
src/NLog/Internal/XmlParser.cs
Rewrites TryParseUnicodeValue/TryParseUnicodeValueHex to init unicode to 0, use local current char, enforce digit/hex ranges and max Unicode value, and strengthen error handling.
Start/End element, attributes, CDATA, PI
src/NLog/Internal/XmlParser.cs
Start-element and attribute reads now default name/attributes to null on failure, attribute parsing invoked via discarded assignment (_ = TryReadAttributes(...)), CDATA/inner-text handling simplified, and processing-instruction flow preserved.
Error messages & EOF handling
src/NLog/Internal/XmlParser.cs
Standardizes and tightens exception messages (including line info), updates unexpected-end wording, and adjusts EOF/error text for XmlParserElement.Current.
Cosmetic / cleanup
src/NLog/Internal/XmlParser.cs
Removes unused locals/assignments, simplifies return paths, and makes internal refactors without altering public API surface.

Sequence Diagram(s)

sequenceDiagram
  participant Loader as Parser.LoadDocument
  participant Src as _xmlSource
  participant P as XmlParser

  rect `#f3f7ff`
    Loader->>P: Start parsing
    P->>Src: MoveNext / Peek (read chars)
    alt Start element
      P->>P: TryReadStartElement()
      P->>P: TryReadName() — local `chr` loop
      P->>P: _ = TryReadAttributes()
    end
    alt Unicode/Hex sequence
      P->>P: TryParseUnicodeValue / TryParseUnicodeValueHex()
      note right of P `#e9f9ee`: local char, explicit digit/hex checks\nenforce max Unicode
    end
    alt End-tag / token validation
      P->>P: foreach char → validate (no index arithmetic)
    end
    P->>Loader: Return parsed document or throw (errors include line info)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Check Unicode/hex boundary checks and max-value enforcement.
  • Verify foreach end-tag/token loops preserve previous edge-case semantics.
  • Confirm error-message formatting includes correct line numbers and context.
  • Ensure _ = TryReadAttributes(...) usage doesn't hide needed return-value handling.

Poem

🐰 I hopped through tags both wide and narrow,
Local chars kept snug and bright,
Hexes checked beneath my paw,
Tokens tidy, errors right,
I nibble bugs and dance at night. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a description explaining the refactoring objectives and any behavioral changes or benefits achieved by consolidating Unicode and hex parsing logic.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title specifically references refactoring TryParseUnicodeValue to match TryParseUnicodeValueHex, which aligns with the summary's mention of consolidated character validation and unified Unicode/hex parsing branches.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd2b59d and bedb1b7.

📒 Files selected for processing (1)
  • src/NLog/Internal/XmlParser.cs (18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/NLog/Internal/XmlParser.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: task-list-completed

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 and usage tips.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 8, 2025

@snakefoot snakefoot merged commit 9face8b into NLog:dev Nov 8, 2025
5 of 6 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Nov 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant