Review all ifdefs; add net standard 2.1 target #618
Conversation
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant QRGenerator as QRCodeGenerator
participant PngRenderer as PngByteQRCode
participant Pool as ArrayPool<byte>
participant Heap as ManualAllocation
Caller->>QRGenerator: Build QR data / ECC
QRGenerator-->>Caller: QR modules / raw data
Caller->>PngRenderer: Render PNG bytes
alt HAS_SPAN defined
PngRenderer->>Pool: Rent buffers / scanlines
PngRenderer-->>Caller: Compose PNG from pooled buffers
PngRenderer->>Pool: Return buffers
else HAS_SPAN not defined
PngRenderer->>Heap: Allocate scanline arrays on heap
PngRenderer-->>Caller: Compose PNG from allocated arrays
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
QRCoder/QRCodeGenerator.cs (3)
589-597: Build break: BitArray.RightShift is not gated correctly.RightShift is not available on netstandard2.1, net5.0, or net6.0. Guard these calls with NET7_0_OR_GREATER (or keep the loop for HAS_SPAN paths).
Apply this diff:
-#if HAS_SPAN - fStrEcc.RightShift(num); // Shift towards bit 0 -#else +#if NET7_0_OR_GREATER + fStrEcc.RightShift(num); // Shift towards bit 0 +#else for (var i = 0; i < fStrEcc.Length - num; i++) fStrEcc[i] = fStrEcc[i + num]; for (var i = fStrEcc.Length - num; i < fStrEcc.Length; i++) fStrEcc[i] = false; #endif
601-609: Same issue for BitArray.LeftShift.LeftShift is not available on ns2.1/net5/net6. Use NET7_0_OR_GREATER.
Apply this diff:
-#if HAS_SPAN - fStrEcc.LeftShift(num); // Shift away from bit 0 -#else +#if NET7_0_OR_GREATER + fStrEcc.LeftShift(num); // Shift away from bit 0 +#else for (var i = fStrEcc.Length - 1; i >= num; i--) fStrEcc[i] = fStrEcc[i - num]; for (var i = 0; i < num; i++) fStrEcc[i] = false; #endif
1104-1115: Return pooled byte buffer under all paths.GetBytes(plainText, codeBytes) can throw; wrap pooling with try/finally to avoid leaks.
Apply this diff:
- int count = targetEncoding.GetByteCount(plainText); - byte[]? bufferFromPool = null; - Span<byte> codeBytes = (count <= MAX_STACK_SIZE_IN_BYTES) - ? (stackalloc byte[MAX_STACK_SIZE_IN_BYTES]) - : (bufferFromPool = ArrayPool<byte>.Shared.Rent(count)); - codeBytes = codeBytes.Slice(0, count); - targetEncoding.GetBytes(plainText, codeBytes); + int count = targetEncoding.GetByteCount(plainText); + byte[]? bufferFromPool = null; + Span<byte> codeBytes = (count <= MAX_STACK_SIZE_IN_BYTES) + ? (stackalloc byte[MAX_STACK_SIZE_IN_BYTES]) + : (bufferFromPool = ArrayPool<byte>.Shared.Rent(count)); + codeBytes = codeBytes.Slice(0, count); + try + { + targetEncoding.GetBytes(plainText, codeBytes); + } + finally + { + // no-op for stackalloc; pooled buffer is returned after BitArray conversion below + } @@ - if (bufferFromPool != null) - ArrayPool<byte>.Shared.Return(bufferFromPool); + if (bufferFromPool != null) + ArrayPool<byte>.Shared.Return(bufferFromPool);Also applies to: 1135-1138
🧹 Nitpick comments (4)
QRCoder/QRCoder.csproj (3)
9-9: HAS_SPAN define condition looks right.Using IsTargetFrameworkCompatible(netcoreapp2.1) OR explicit netstandard2.1 correctly lights up Span/ArrayPool paths for netcoreapp2.1+, net5+, net6+, and ns2.1. Consider adding a short comment explaining why ns2.1 needs the explicit OR.
48-50: CodePages for ns1.3/ns2.0/ns2.1 — OK.Works for ECI encodings. Optionally bump to a recent version if you plan a package update soon.
Would you like a quick script to check current vulnerable versions before release?
52-54: System.Drawing.Common references — check platform intent.Including 5.0.3 for net5.0 (all) and ns2.0/ns2.1, and 6.0.0 for net6.0-windows aligns with Windows-only guidance for .NET 6+. If you expect non‑Windows net5 usage, keep in mind libgdiplus requirements at runtime.
QRCoder/QRCodeGenerator/Polynom.cs (1)
200-212: Switch to HAS_SPAN for ArrayPool — LGTM.Fully-qualified System.Buffers usage avoids a using-directive and keeps the non‑HAS_SPAN fallback intact. Optional: Return with clearArray:false explicitly to document intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
QRCoder/Attributes/NotNullWhenAttribute.cs(1 hunks)QRCoder/PngByteQRCode.cs(4 hunks)QRCoder/QRCodeGenerator.cs(13 hunks)QRCoder/QRCodeGenerator/Polynom.cs(1 hunks)QRCoder/QRCoder.csproj(2 hunks)
⏰ 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: build
🔇 Additional comments (7)
QRCoder/Attributes/NotNullWhenAttribute.cs (1)
1-1: Correct guard to avoid duplicate attribute on netstandard2.1+.Excluding NETSTANDARD2_1 (in addition to NETCOREAPP) prevents type redefinition where the BCL already ships this attribute. LGTM.
QRCoder/QRCoder.csproj (1)
4-4: Add netstandard2.1 target — LGTM.This fills the gap between netstandard2.0 and net5+, enabling Span APIs without moving to a runtime TFM.
QRCoder/PngByteQRCode.cs (2)
1-3: Scoped System.Buffers import — LGTM.Guarded import matches HAS_SPAN usage below.
100-106: ArrayPool allocation — OK.Rent/clear pattern is fine. Returning is handled above.
QRCoder/QRCodeGenerator.cs (3)
2-4: Scoped System.Buffers import — LGTM.
1013-1038: Span-based parsing for numeric mode — LGTM.Appropriate use of int.Parse(ReadOnlySpan) under HAS_SPAN, with non‑SPAN fallback preserved.
700-707: Pooled ECC buffers are returned.
ReturnList invokes ArrayPool.Shared.Return(item.ECCWords.Array!) under #if NETCOREAPP, so rented ECC arrays are properly returned to the pool.
Note: .NET Standard 2.1 is tested via the existing .NET Core 3.1 tests
Summary by CodeRabbit
New Features
Performance
Chores