-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Append4DigitsZeroPadded - Use Span instead of multiple StringBuilder Append #5983
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.
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
Conversation
WalkthroughOptimizes Append4DigitsZeroPadded in StringBuilderExt by adding a Span-based append path for newer frameworks under conditional compilation and retains the original per-digit appends as fallback. Also rewrites a numeric range check expression. The changes are applied in two places within the file. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/NLog/Internal/StringBuilderExt.cs (2)
424-435: Prefer AppendSpan (no temp buffer) and avoid C# 12 collection expressionsThe collection expression
Span<char> buffer = [ ... ];requires C# 12 and might break older LangVersion targets. Also, usingAppendSpan(4)avoids the extra copy from a temporary span into StringBuilder.Proposed replacement within this block:
- Span<char> buffer = - [ - (char)(((number / 1000) % 10) + '0'), - (char)(((number / 100) % 10) + '0'), - (char)(((number / 10) % 10) + '0'), - (char)((number % 10) + '0'), - ]; - builder.Append(buffer); // Single Append instead of many + var dest = builder.AppendSpan(4); + dest[0] = (char)(((number / 1000) % 10) + '0'); + dest[1] = (char)(((number / 100) % 10) + '0'); + dest[2] = (char)(((number / 10) % 10) + '0'); + dest[3] = (char)((number % 10) + '0');If
AppendSpanisn’t available for all targeted TFMs, alternatively use a non-C#12 form:- Span<char> buffer = [ /* digits */ ]; - builder.Append(buffer); + Span<char> buffer = stackalloc char[4]; + buffer[0] = (char)(((number / 1000) % 10) + '0'); + buffer[1] = (char)(((number / 100) % 10) + '0'); + buffer[2] = (char)(((number / 10) % 10) + '0'); + buffer[3] = (char)((number % 10) + '0'); + builder.Append(buffer);
415-443: Clarify input domain (expects 0..9999); negative values render incorrect digitsThe fallback digit math will produce garbage for negatives (e.g., -1 yields '/000'). Current call sites seem to pass non-negative values (year, fraction), but make the contract explicit.
Consider adding a debug-time guard near the start of the method:
internal static void Append4DigitsZeroPadded(this StringBuilder builder, int number) { + System.Diagnostics.Debug.Assert(number >= 0 && number < 10000, "Append4DigitsZeroPadded expects 0..9999");Alternatively, document the assumption in XML comments if you want zero runtime impact in Release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/NLog/Internal/StringBuilderExt.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Internal/StringBuilderExt.cs (1)
src/NLog/LayoutRenderers/LongDateLayoutRenderer.cs (1)
Append(61-80)
🔇 Additional comments (1)
src/NLog/Internal/StringBuilderExt.cs (1)
418-421: LGTM: fast-path for 4-digit valuesThe 4-digit range check keeps yyyy fast and avoids the zero-padding path. Looks good.
|



No description provided.