Skip to content

XmlLayout - Changed to foreach for better release optimizations #6018

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
Merged
snakefoot merged 1 commit into from
Oct 5, 2025
Merged

XmlLayout - Changed to foreach for better release optimizations #6018

snakefoot merged 1 commit into from
Oct 5, 2025

Conversation

@snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Oct 5, 2025

Release-mode can optimize bounds-checking on newer NET platforms with foreach.

@coderabbitai
Copy link

coderabbitai bot commented Oct 5, 2025 •
edited
Loading

Walkthrough

Updates stringification across JSON and core Layout classes, altering separators and outputs for attribute rendering. Refactors XML element base to use private backing lists for elements and attributes, updating internal loops and checks accordingly while keeping public read-only accessors.

Changes

Cohort / File(s) Summary
JSON layout stringify tweaks
src/NLog/Layouts/JSON/JsonLayout.cs		 Adjust ToString: use name=value pairs via ToStringWithNestedItems when attributes exist; return class name with IncludeEventProperties flag when no attributes; otherwise return class name.
Layout base stringify helper
src/NLog/Layouts/Layout.cs		 ToStringWithNestedItems: when items exist, use ": " before joined names (instead of "="); when empty, return type name instead of base.ToString().
XML element backing fields refactor
src/NLog/Layouts/XML/XmlElementBase.cs		 Introduce private List backing fields for Elements/Attributes; properties now return backing fields. Replace internal usage to reference _elements/_attributes across initialization, pre-calculation, rendering, and ToString helpers. Public API shape remains read-only accessors.

Sequence Diagram(s)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I twitch my ears at name=value flair,
JSON shines neatly, pairs everywhere.
XML hides lists in burrows tight,
Backing fields snug, out of sight.
Layouts whisper: “Type names, colon bright.”
I thump approval—logs hop just right! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title claims the PR changes XmlLayout to use foreach for performance optimizations, but the actual changes focus on refactoring ToString implementations in JsonLayout and Layout and introducing private backing fields in XmlElementBase, with no mention of foreach loops or related performance work. Therefore the title does not accurately summarize the main changes. Please update the title to clearly reflect the primary modifications, such as refactoring ToString methods in JSON and XML layout classes and introducing backing fields for XML elements, rather than referencing unrelated foreach optimizations.
Description Check ⚠️ Warning The description refers to optimizing bounds-checking via foreach in release mode, but the diff shows no changes to loops or performance-oriented code; instead it adjusts string representations and refactors backing fields in layout classes, making the description unrelated to the actual work. Please revise the description to summarize the actual changes, for example noting the updates to ToString behavior in JsonLayout and Layout and the introduction of private backing fields in XmlElementBase.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ 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 2ea7732 and 85873b4.

📒 Files selected for processing (3)
  • src/NLog/Layouts/JSON/JsonLayout.cs (1 hunks)
  • src/NLog/Layouts/Layout.cs (1 hunks)
  • src/NLog/Layouts/XML/XmlElementBase.cs (8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/NLog/Layouts/Layout.cs (1)
src/NLog/Internal/StringHelpers.cs (1)
  • Join (157-164)
src/NLog/Layouts/JSON/JsonLayout.cs (2)
src/NLog/Layouts/Layout.cs (7)
  • ToStringWithNestedItems (504-512)
  • Layout (105-108)
  • Layout (116-119)
  • Layout (127-144)
  • Layout (149-155)
  • Layout (163-170)
  • Layout (382-412)
src/NLog/Layouts/XML/XmlElementBase.cs (1)
  • ToString (800-814)
src/NLog/Layouts/XML/XmlElementBase.cs (4)
src/NLog/Layouts/XML/XmlAttribute.cs (3)
  • XmlAttribute (56-56)
  • XmlAttribute (63-63)
  • XmlAttribute (71-77)
src/NLog/Layouts/JSON/JsonLayout.cs (2)
  • Layout (50-510)
  • ToString (501-509)
src/NLog/Layouts/Layout.cs (7)
  • Layout (105-108)
  • Layout (116-119)
  • Layout (127-144)
  • Layout (149-155)
  • Layout (163-170)
  • Layout (382-412)
  • ToStringWithNestedItems (504-512)
src/NLog/Layouts/XML/XmlLayout.cs (1)
  • Layout (45-100)

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 Oct 5, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
39.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@snakefoot snakefoot merged commit 631ed06 into NLog:dev Oct 5, 2025
3 of 6 checks passed
@snakefoot snakefoot added this to the 6.0.5 milestone Oct 5, 2025
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

performance size/M xml-layout

Projects

None yet

Milestone

6.0.5

Development

Successfully merging this pull request may close these issues.

1 participant

@snakefoot