Skip to content

CompoundLayout - Changed to foreach for better release optimizations #6015

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

CompoundLayout - Changed to foreach for better release optimizations #6015

snakefoot merged 1 commit into from
Oct 5, 2025

Conversation

@snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Oct 5, 2025 •
edited
Loading

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

@snakefoot snakefoot added this to the 6.0.5 milestone Oct 5, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 5, 2025 •
edited
Loading

Walkthrough

Refactors CompoundLayout by replacing the public mutable Layouts with a private List exposed via a read-only property, removes the public constructor, and updates all internal references to use the private backing field for initialization and iteration.

Changes

Cohort / File(s) Summary
CompoundLayout refactor
src/NLog/Layouts/CompoundLayout.cs		 Replaced public setter/property for Layouts with a read-only property backed by a private List initialized inline; removed the public constructor; redirected all initialization, iteration, and modification to the private _layouts field and expression-bodied getter.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Hop hop! I tidied up the nest,
Hid the lists, made access blessed.
Constructors hopped right out of sight,
A private burrow keeps things tight.
With layouts lined in tidy rows,
This rabbit’s code more smoothly flows. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The pull request title mentions changing to foreach for release optimizations, but the actual changes refactor CompoundLayout by replacing its public setter property with a read-only expression-bodied property backed by a private List and removing the public constructor, with no adjustments to foreach loops or release behavior, making the title misleading and off-target. Please update the title to clearly reflect the main change, for example “Make CompoundLayout.Layouts read-only with private backing field and remove public setter,” which accurately describes the refactor.
Description Check ⚠️ Warning The pull request has no author-provided description, leaving reviewers with no summary or context for the refactor of CompoundLayout, and therefore no meaningful information about the purpose or scope of the changes. Please add a concise description outlining the key changes, such as converting the Layouts property to a read-only expression-bodied property backed by a private List and removing the public constructor, to help reviewers understand the intent and scope of this pull request.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
✨ 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 afb553c and 220d461.

📒 Files selected for processing (1)
  • src/NLog/Layouts/CompoundLayout.cs (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Layouts/CompoundLayout.cs (1)
src/NLog/Layouts/Layout.cs (13)
  • Layout (105-108)
  • Layout (116-119)
  • Layout (127-144)
  • Layout (149-155)
  • Layout (163-170)
  • Layout (382-412)
  • InitializeLayout (430-433)
  • Initialize (323-346)
  • Render (207-223)
  • Render (231-248)
  • CloseLayout (438-440)
  • Close (417-425)
  • ToStringWithNestedItems (504-512)
🔇 Additional comments (2)
src/NLog/Layouts/CompoundLayout.cs (2)

65-103: Foreach optimization achieved by iterating concrete type.

Using _layouts (List) directly instead of Layouts (IList) enables the compiler to leverage List<Layout>.Enumerator, a struct enumerator that avoids heap allocation and interface dispatch overhead. This optimization is particularly valuable in logging hot paths like RenderFormattedMessage.

59-60: Approve removal of the setter — no internal assignments of Layouts were found.

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 Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

@snakefoot snakefoot merged commit aaeb716 into NLog:dev Oct 5, 2025
5 of 6 checks passed
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/S

Projects

None yet

Milestone

6.0.5

Development

Successfully merging this pull request may close these issues.

1 participant

@snakefoot