Optimize alphanumeric encoder and galois field constants #684
Conversation
…ength calculations and writing methods
…hanumericDataSegment
…n AlphanumericEncoder
…stency and clarity
… improve performance
📝 WalkthroughWalkthroughWidened multiple previously-private members to internal, refactored AlphanumericEncoder to use a compact Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant QRCodeGenerator
participant AlphanumericEncoder
Caller->>QRCodeGenerator: GetEncodingFromPlaintext(text, forceUtf8)
QRCodeGenerator->>QRCodeGenerator: iterate characters
loop per character c
QRCodeGenerator->>AlphanumericEncoder: CanEncode(c)
alt CanEncode == true
QRCodeGenerator-->QRCodeGenerator: treat as Alphanumeric candidate
else
QRCodeGenerator-->QRCodeGenerator: evaluate Numeric/Byte modes
end
end
Note right of AlphanumericEncoder #e6f7ff: CanEncode uses direct `_map` lookup (span/array)
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 (beta)
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: 1
🧹 Nitpick comments (3)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (3)
10-30: C# 12 collection expressions may limit TFM compatibility; consider a fallback or pin LangVersion explicitlyThe
[_ ... ]collection expression for bothReadOnlySpan<byte>andbyte[]requires C# 12. If any target uses an older language version, builds will fail. Either ensure LangVersion 12 across all TFMs, or refactor to a single backing array with a span view to avoid C# 12:
- Define one backing array once.
- Expose it as
ReadOnlySpan<byte>when HAS_SPAN, or as the array otherwise.Example (conceptual):
private static readonly byte[] s_map = new byte[] { /* same bytes as currently listed */ }; #if HAS_SPAN private static ReadOnlySpan<byte> _map => s_map; #else private static readonly byte[] _map = s_map; #endifPlease confirm CI uses C# 12 for all targets if you keep collection expressions.
32-35: Update summary; clarify semantics of CanEncodeXML summary says “non-digit character”, but the method checks any character. Also consider using a char literal for readability.
-/// Checks if a non-digit character is present in the alphanumeric encoding table. +/// Returns true if the character is encodable in QR alphanumeric mode. -public static bool CanEncode(char c) => c <= 90 && _map[c] != 255; +public static bool CanEncode(char c) => c <= 'Z' && _map[c] != 255;
77-82: Defensive checks to catch misuse in debug builds; minor micro-optsAdd Debug.Assert guards to verify inputs are alphanumeric when this path is used. Also cache
_maplocally to avoid repeated property access under HAS_SPAN.-while (count >= 2) +var map = _map; +while (count >= 2) { - var dec = _map[plainText[index++]] * 45 + _map[plainText[index++]]; + var c1 = plainText[index++]; + var c2 = plainText[index++]; +#if DEBUG + System.Diagnostics.Debug.Assert(CanEncode(c1) && CanEncode(c2), "Non‑alphanumeric input passed to AlphanumericEncoder."); +#endif + var dec = map[c1] * 45 + map[c2]; codeIndex = DecToBin(dec, 11, codeText, codeIndex); count -= 2; } ... if (count > 0) { - codeIndex = DecToBin(_map[plainText[index]], 6, codeText, codeIndex); + var c = plainText[index]; +#if DEBUG + System.Diagnostics.Debug.Assert(CanEncode(c), "Non‑alphanumeric input passed to AlphanumericEncoder."); +#endif + codeIndex = DecToBin(map[c], 6, codeText, codeIndex); }Also applies to: 85-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/QRCodeGenerator.cs(1 hunks)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs(3 hunks)QRCoderTests/QRGeneratorTests.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
QRCoder/QRCodeGenerator.cs (1)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
AlphanumericEncoder(8-92)CanEncode(35-35)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (1)
QRCoder/QRCodeGenerator.cs (1)
DecToBin(874-884)
⏰ 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). (6)
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Core 3.1
- GitHub Check: additional-tests
🔇 Additional comments (4)
QRCoder/QRCodeGenerator.cs (1)
756-762: LGTM: mode detection correctly delegates to new CanEncodeSwitching to AlphanumericEncoder.CanEncode(c) aligns with the new map and preserves behavior.
QRCoderTests/QRGeneratorTests.cs (3)
418-427: LGTM: map -> dictionary materializationEfficient and clear; sentinel 255 is handled correctly.
432-441: Good coverage: validates CanEncode for all Unicode code pointsSolid exhaustive check against the constructed map.
444-448: LGTM: deterministic ordering for expected mapping stringOrdering by mapped value preserves the original assertion semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (1)
QRCoder/QRCodeGenerator.cs (1)
DecToBin(874-884)
🔇 Additional comments (2)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
10-32: Excellent optimization approach.The use of
ReadOnlySpan<byte>with an inline array literal for HAS_SPAN targets leverages the compiler's ability to embed this data in the assembly's read-only section, eliminating runtime allocation. The fallback tobyte[]for older frameworks maintains compatibility. The mapping itself is correct for QR code alphanumeric mode (0-9, A-Z, space, and special characters).
74-84: Direct array indexing delivers the performance win.Replacing dictionary lookups with direct
_mapindexing at lines 80 and 89 is the key optimization. The absence of bounds checking is intentional and appropriate for a hot path, as input validation is expected at the API boundary viaCanEncode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoderTests/QRGeneratorTests.cs (1)
3-3: Verify the import change is intentional.The import changed from
System.ReflectiontoSystem.Linq.Expressions, butSystem.Linq.Expressionsdoesn't appear to be used in the updated code. Consider removing this unused import or reverting to remove both if neither is needed.Apply this diff if the import is unused:
-using System.Linq.Expressions;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
QRCoder/PayloadGenerator.cs(2 hunks)QRCoder/QRCodeGenerator.cs(2 hunks)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs(3 hunks)QRCoder/QRCodeGenerator/EncodingMode.cs(1 hunks)QRCoder/QRCodeGenerator/GaloisField.cs(1 hunks)QRCoder/QRCoder.csproj(1 hunks)QRCoderTests/PayloadGeneratorTests/IbanTests.cs(5 hunks)QRCoderTests/QRGeneratorTests.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoder/QRCodeGenerator/AlphanumericEncoder.cs
🧰 Additional context used
🧬 Code graph analysis (4)
QRCoder/QRCodeGenerator/EncodingMode.cs (1)
QRCoder/QRCodeGenerator.cs (1)
EncodingMode(749-764)
QRCoder/QRCodeGenerator.cs (1)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
AlphanumericEncoder(8-94)CanEncode(37-37)
QRCoderTests/QRGeneratorTests.cs (3)
QRCoder/QRCodeGenerator.cs (2)
QRCodeGenerator(14-1028)QRCodeGenerator(19-21)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (3)
QRCodeGenerator(3-95)AlphanumericEncoder(8-94)CanEncode(37-37)QRCoder/QRCodeGenerator/GaloisField.cs (2)
QRCodeGenerator(5-51)GaloisField(16-50)
QRCoderTests/PayloadGeneratorTests/IbanTests.cs (1)
QRCoder/PayloadGenerator.cs (3)
PayloadGenerator(10-150)IsValidIban(17-44)IsValidQRIban(51-62)
⏰ 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). (6)
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: additional-tests
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
🔇 Additional comments (10)
QRCoder/PayloadGenerator.cs (1)
17-17: LGTM! Visibility changes enable reflection-free testing.The visibility changes from
privatetointernalforIsValidIbanandIsValidQRIbanalign with the PR's objective to eliminate reflection-based testing. Combined with theInternalsVisibleToattribute, tests can now directly access these methods.Also applies to: 51-51
QRCoderTests/PayloadGeneratorTests/IbanTests.cs (1)
10-10: LGTM! Cleaner test implementation.Replacing reflection-based invocation with direct method calls improves readability, maintainability, and performance. This change aligns perfectly with the PR's goal of eliminating reflection in tests.
Also applies to: 20-20, 30-30, 40-40, 50-50
QRCoder/QRCoder.csproj (1)
19-21: LGTM! Standard approach for test access to internals.Adding
InternalsVisibleTofor the test assembly is the standard .NET practice for exposing internal members to tests without making them public. This enables the reflection-free test approach used throughout the PR.QRCoder/QRCodeGenerator/EncodingMode.cs (1)
8-8: LGTM! Consistent visibility enhancement.The change from
privatetointernalfor theEncodingModeenum aligns with the broader internal visibility enhancements across the codebase, enabling test access viaInternalsVisibleTo.QRCoder/QRCodeGenerator/GaloisField.cs (1)
16-16: LGTM! Enables direct test access to Galois field internals.The visibility changes for
GaloisFieldand its lookup tables support reflection-free testing, consistent with the PR's objectives.Also applies to: 18-19
QRCoder/QRCodeGenerator.cs (2)
749-749: LGTM! Visibility change supports reflection-free testing.The change from
privatetointernalforGetEncodingFromPlaintextenables direct test access, consistent with the PR's objectives.
759-759: LGTM! Refined API with clearer naming.The change from
CanEncodeNonDigittoCanEncodeimproves the API clarity. The method now checks all alphanumeric characters (both digits and non-digits), which is semantically correct since the digit check on line 756 handles the numeric-only case first.QRCoderTests/QRGeneratorTests.cs (3)
16-16: LGTM! Direct access eliminates reflection complexity.Direct access to
GaloisField._galoisFieldByExponentAlphaandGaloisField._galoisFieldByIntegerValueviaInternalsVisibleTois cleaner and more performant than the previous reflection-based approach.Also applies to: 24-24
379-409: LGTM! Improved test with better coverage.The rewritten
validate_alphanumencdicttest is significantly improved:
- Direct access to
AlphanumericEncoder._mapeliminates reflection complexity- Added comprehensive
CanEncodeverification (lines 392-400) validates all Unicode scalar values- Cleaner code using LINQ for ordering
This addresses the past review concern about reflection failures on different .NET versions.
414-414: LGTM! Direct method calls improve test clarity.Replacing reflection with direct calls to
QRCodeGenerator.GetEncodingFromPlaintextmakes the tests more readable and maintainable.Also applies to: 423-423, 432-432, 441-441
Shane32
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoderTests/QRGeneratorTests.cs (1)
389-397: Consider if checking all 65,536 chars is necessary.The loop verifies
CanEncodefor every possiblecharvalue, which is thorough but may slow down the test suite. If performance becomes an issue, you could test a representative subset (e.g., all chars ≤ 90 plus spot-checks for chars > 90).That said, comprehensive coverage is valuable in tests, so this is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
QRCoder/QRCodeGenerator/GaloisField.cs(1 hunks)QRCoderTests/QRGeneratorTests.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/QRGeneratorTests.cs (3)
QRCoder/QRCodeGenerator/GaloisField.cs (2)
QRCodeGenerator(5-62)GaloisField(16-61)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (3)
QRCodeGenerator(3-95)AlphanumericEncoder(8-94)CanEncode(37-37)QRCoder/QRCodeGenerator.cs (2)
QRCodeGenerator(14-1028)QRCodeGenerator(19-21)
⏰ 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: additional-tests
🔇 Additional comments (7)
QRCoder/QRCodeGenerator/GaloisField.cs (1)
16-30: LGTM! Efficient type optimization and appropriate visibility for testing.The changes are correct:
internalvisibility enables direct test access via InternalsVisibleTo (avoiding reflection)byte[]is more memory-efficient thanint[]since all values are 0–255ReadOnlySpan<byte>when available leverages read-only data section inlining for performanceQRCoderTests/QRGeneratorTests.cs (6)
13-13: Direct access via InternalsVisibleTo—simpler and more maintainable.This resolves the prior reflection concerns for .NET Core 3.1–7.0 by using direct member access instead of building expression trees.
21-21: Direct access via InternalsVisibleTo—simpler and more maintainable.
411-411: Direct call via InternalsVisibleTo—simpler and more maintainable.
420-420: Direct call via InternalsVisibleTo—simpler and more maintainable.
429-429: Direct call via InternalsVisibleTo—simpler and more maintainable.
438-438: Direct call via InternalsVisibleTo—simpler and more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Directory.Build.targets (1)
9-9: Update the comment to reflect the actual behavior.The comment states "packable projects" but the condition on line 10 no longer checks
IsPackable, so signing now applies to all Release builds.Apply this diff to update the comment:
- <!-- Strong name signing for Release builds of packable projects --> + <!-- Strong name signing for Release builds -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Directory.Build.targets(1 hunks)QRCoder/QRCoder.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoder/QRCoder.csproj
⏰ 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). (6)
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET Core 2.1
- GitHub Check: additional-tests
🔇 Additional comments (1)
Directory.Build.targets (1)
10-13: Verify signing of all Release assemblies: Removing theIsPackablecondition will now strong-name sign every Release build (including test projects). Rundotnet build -c Releaseanddotnet test -c Releaseto confirm no strong-name mismatches or deployment issues.
| <ItemGroup Condition="'$(Configuration)' == 'Release'"> | ||
| <InternalsVisibleTo Include="QRCoderTests, PublicKey=002400000480000094000000060200000024000052534131000400000100010071e74d3f6dcbba7726fcbb7ad05a2dfa3b8e13f9bf2938c05de266a83a7f3c95201b7cdc9e1f88842093ece868c3ec7874e3b8907008763c85711bf0e2e9757a3068385380a757bd52fa77248f227f602b0785e040756ef42dabc7de7f8376847c71b3f20356a9176cc88067583ee3501e61f8aa07bdd70f41418986fb79a1a3" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(Configuration)' != 'Release'"> | ||
| <InternalsVisibleTo Include="QRCoderTests" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
What a pain! A strongly-named assembly can only reference another strongly-named assembly. If I make it conditional upon release, then the solution won't build in release mode because the tests can't see the internal members. So I had to make the test project strongly named, and extract the public key to put in the prop here, and it had to be conditional because otherwise in debug mode it couldn't find the assembly with the strong name...
There was a problem hiding this comment.
A strongly-named assembly can only reference another strongly-named assembly
Yeah, that virality sucks.
be conditional because otherwise in debug mode
When it's strong named in DEBUG and RELEASE it should work in all cases?! So no conditional is needed.
But it's fine as is too.
There was a problem hiding this comment.
Codecov can’t inject its code into a strongly named assembly. So to get the coverage reports working I had to disable strong name signing.
There was a problem hiding this comment.
Ah, I didn't think about this. Thanks for clarification.
| { | ||
| var nextPos = startPos + numericCount; | ||
| if (nextPos < text.Length && !IsAlphanumericNonDigit(text[nextPos])) | ||
| if (nextPos < text.Length && !IsAlphanumeric(text[nextPos])) |
There was a problem hiding this comment.
The next character is known to not be a digit, so it doesn't matter if this checks against IsAlphanumeric or IsAlphanumericNonDigit. Since the new optimzation is faster with the former, it's changed here and below.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/QRCodeGenerator/OptimizedDataSegment.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/QRCodeGenerator/OptimizedDataSegment.cs (1)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (1)
AlphanumericEncoder(8-94)
⏰ 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). (6)
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: additional-tests
🔇 Additional comments (2)
QRCoder/QRCodeGenerator/OptimizedDataSegment.cs (2)
332-332: LGTM: Good refactoring to centralize alphanumeric checks.Delegating to
AlphanumericEncoder.CanEncode(c)establishes a single source of truth for alphanumeric character validation, improving maintainability.
178-178: LGTM: Correct bug fix for exclusive Byte subset check.The change from
!IsAlphanumericNonDigitto!IsAlphanumericcorrectly identifies characters in the "exclusive subset of the Byte character set" (characters that can only be encoded in Byte mode, not in Alphanumeric or Numeric modes). The previous logic would have incorrectly treated digits as byte-only characters.
| <ItemGroup Condition="'$(Configuration)' == 'Release'"> | ||
| <InternalsVisibleTo Include="QRCoderTests, PublicKey=002400000480000094000000060200000024000052534131000400000100010071e74d3f6dcbba7726fcbb7ad05a2dfa3b8e13f9bf2938c05de266a83a7f3c95201b7cdc9e1f88842093ece868c3ec7874e3b8907008763c85711bf0e2e9757a3068385380a757bd52fa77248f227f602b0785e040756ef42dabc7de7f8376847c71b3f20356a9176cc88067583ee3501e61f8aa07bdd70f41418986fb79a1a3" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(Configuration)' != 'Release'"> | ||
| <InternalsVisibleTo Include="QRCoderTests" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
A strongly-named assembly can only reference another strongly-named assembly
Yeah, that virality sucks.
be conditional because otherwise in debug mode
When it's strong named in DEBUG and RELEASE it should work in all cases?! So no conditional is needed.
But it's fine as is too.
|
|
||
| <!-- Strong name signing for Release builds of packable projects --> | ||
| <PropertyGroup Condition="'$(Configuration)' == 'Release' AND '$(IsPackable)' == 'true'"> | ||
| <PropertyGroup Condition="'$(Configuration)' == 'Release'"> |
There was a problem hiding this comment.
This is what I referred to. Just make it unconditional signed.
But we won't change that often (never), so fine as is too.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests