V10.1.0/api improvements and features #134
Conversation
…zations for 32/64-bit
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces comprehensive performance optimizations, multi-precision hashing support, and benchmarking infrastructure alongside new post-configuration capability for parameter objects. Updates core data structures, security implementations, package dependencies, and CI workflows while adding extensive test coverage and performance reports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the DateSpan structure and FowlerNollVoHash implementation to improve code maintainability and performance. The changes extract complex date difference calculations into helper methods, implement a robust FNV-64 hash for DateSpan, and extend FowlerNollVoHash to support multiple bit sizes (32, 64, 128, 256, 512, 1024) with optimized multi-word arithmetic for larger hashes.
- Refactored
DateSpanconstructor logic into dedicated helper methods for better readability - Replaced
DateSpan.GetHashCode()with FNV-64 implementation and calendar type identification - Extended
FowlerNollVoHashto support multiple bit sizes with specialized computation methods
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Cuemon.Core/DateSpan.cs | Refactored date calculation logic into helper methods; implemented FNV-64 hash code generation with calendar ID tracking |
| src/Cuemon.Core/Security/FowlerNollVoHash.cs | Added multi-bit size support (32, 64, 128, 256, 512, 1024) with specialized computation methods and multi-word arithmetic |
| test/Cuemon.Core.Tests/DateSpanTest.cs | Added hash code stability tests; removed hard-coded hash code assertions from existing tests |
| test/Cuemon.Core.Tests/Security/FowlerNollVoHashTest.cs | New comprehensive test file covering various bit sizes, algorithms, and endianness configurations |
| src/Cuemon.Core/GlobalSuppressions.cs | Removed suppressions for cognitive complexity and unused assignments, reflecting improved code quality |
| .github/copilot-instructions.md | Added comprehensive guidelines for unit testing and XML documentation in the Cuemon codebase |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
test/Cuemon.Core.Tests/Security/FowlerNollVoHashTest.cs (2)
49-115: Solid 32/64-bit FNV behavior coverage; consider adding LittleEndian non-empty casesThe 32-bit and 64-bit tests:
- Correctly use standard FNV primes/offset bases.
- Cross-check both Fnv1a and Fnv1 against independent reference implementations that mirror the production loops and endianness handling.
- Verify empty-input behavior for both default BigEndian and explicitly configured LittleEndian (32-bit).
That gives strong confidence that the new
ComputeHash32/ComputeHash64logic andFowlerNollVoOptions.Algorithm/ByteOrderinterplay are correct.Optional improvement: Add non-empty LittleEndian expectation tests for 32- and/or 64-bit (e.g.,
hellowithByteOrder = Endianness.LittleEndian) to guard against future regressions in the manual byte layout code, similar to what you do for the empty-input case.If you want to see exact reference values to lock into those extra assertions, you can reuse
IndependentFNV32/IndependentFNV64withbyteOrder = Endianness.LittleEndianinside new tests.
117-187: Verify that LittleEndian coverage should be added to multi-word testsThe review comment is accurate. Current state:
- 32-bit tests: Explicit LittleEndian variant exists (line 65:
ComputeHash32_EmptyInput_ReturnsOffsetBasis_LittleEndianWhenConfigured)- 64-bit tests: No LittleEndian variant found
- Multi-word tests: Both tests (lines 118 and 170) use default BigEndian only, despite the empty-input test being named
WithSelectedEndiannessThe constructor pattern for endianness configuration already works (demonstrated by the 32-bit test at line 69:
o => o.ByteOrder = Endianness.LittleEndian), and the helper methodsIndependentFNVMultiWordalready support both endianness paths. Adding an explicit LittleEndian variant to at least one multi-word test would close this coverage gap and align with the 32-bit test pattern..github/copilot-instructions.md (2)
68-117: Fix mismatched summary inDateSpanTestexample to avoid confusionIn the “Example Test” snippet, the XML summary says:
/// <summary> /// Tests for the <see cref="DefaultCommand"/> class. /// </summary> public class DateSpanTest : TestThe class is
DateSpanTest, not a test forDefaultCommand, and the rest of the example clearly focuses onDateSpan. This mismatch could be confusing for contributors using this as a template.Consider aligning the summary and cref, e.g.:
- /// <summary> - /// Tests for the <see cref="DefaultCommand"/> class. - /// </summary> - public class DateSpanTest : Test + /// <summary> + /// Tests for the <see cref="DateSpan"/> class. + /// </summary> + public class DateSpanTest : Test
144-151: Address markdownlint MD003 (heading style) or configure an exception
markdownlint-cli2reports MD003 at Line 144 (“Heading style: Expected: atx; Actual: setext”). This is likely triggered by the second YAML front-matter block and subsequent heading style.Two options:
- Adjust the second document’s top-level heading to clearly use atx style and ensure there isn’t a setext-style underline nearby, or
- Configure markdownlint to ignore this particular heading or file if the current format is intentional.
Given this is purely stylistic, it’s safe to defer, but fixing it will keep automated lint runs clean.
src/Cuemon.Core/DateSpan.cs (1)
203-213: VerifyGetWeeksbehavior for short ranges (≤ 7 days)With the new
GetWeekslogic, any span whererange.Days <= 7yieldsweeks == 2(becausetotalDaysbecomes 1 or 2 and1 + ((totalDays + 6) / 7)evaluates to 2), including the degenerate case of_lower == _upper. Please confirm whether “at least 2 calendar weeks” is the intended semantic; if the intent is “number of calendar weeks spanned” or “weeks-equivalent duration,” you may want a simpler and more explicit formulation for 0–7 day ranges (e.g., returning 1 for any non-empty span and possibly 0 for an empty span).test/Cuemon.Core.Tests/DateSpanTest.cs (1)
1-6: New DateSpan hash-code tests are well-aligned with the revised implementationThe added tests:
- Verify intra-instance stability (
GetHashCode_IsStableForSameInstance).- Check the
Equals/GetHashCodecontract for equal values (GetHashCode_EqualInstances_HaveSameHashCode).- Sanity-check sensitivity to changing boundaries (
GetHashCode_ChangingUpperBoundary_UsuallyChangesHashCode).- Ensure calendar identity is based on calendar type rather than instance (
GetHashCode_TwoSameTypeCalendars_MustBeEqual/GetHashCode_TwoDifferentTypeCalendars_MustBeNotEqual).These collectively exercise the new
_calendarIdmapping and FNV-based hash and give good coverage of the intended semantics. You might consider adding one or two focused tests for short spans with differing time-of-day onceCalculateDifferenceis fixed, but the current hash-focused coverage looks solid.Also applies to: 16-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/copilot-instructions.md(1 hunks)src/Cuemon.Core/DateSpan.cs(9 hunks)src/Cuemon.Core/GlobalSuppressions.cs(0 hunks)src/Cuemon.Core/Security/FowlerNollVoHash.cs(3 hunks)test/Cuemon.Core.Tests/DateSpanTest.cs(2 hunks)test/Cuemon.Core.Tests/Security/FowlerNollVoHashTest.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Cuemon.Core/GlobalSuppressions.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/Cuemon.Core/Security/FowlerNollVoHash.cs (1)
src/Cuemon.Core/Convertible.cs (2)
Convertible(14-487)ReverseEndianness(456-469)
test/Cuemon.Core.Tests/DateSpanTest.cs (1)
src/Cuemon.Core/DateSpan.cs (9)
DateSpan(25-27)DateSpan(34-36)DateSpan(44-100)DateSpan(294-297)DateSpan(305-308)DateSpan(317-320)GetHashCode(223-241)Equals(250-254)Equals(261-265)
test/Cuemon.Core.Tests/Security/FowlerNollVoHashTest.cs (2)
src/Cuemon.Core/Security/FowlerNollVoHash.cs (2)
FowlerNollVoHash(9-249)FowlerNollVoHash(22-46)src/Cuemon.Core/Convertible.cs (2)
Convertible(14-487)ReverseEndianness(456-469)
🪛 markdownlint-cli2 (0.18.1)
.github/copilot-instructions.md
144-144: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
⏰ 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). (5)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
src/Cuemon.Core/Security/FowlerNollVoHash.cs (4)
80-167: Single-word (32/64-bit) FNV implementations correctly handle both algorithms and endianness
ComputeHash32andComputeHash64:
- Implement Fnv1a (
xorthen multiply) and Fnv1 (multiply thenxor) exactly as per the standard FNV definitions.- Run in an
uncheckedcontext, giving the intended modulo-2^32 / modulo-2^64 behavior.- Serialize the final accumulator into 4 or 8 bytes with explicit branches for
Endianness.LittleEndianvs BigEndian, independent of machine endianness.The accompanying tests (
ComputeHash32_NonEmpty_EqualsIndependentImplementation_Fnv1aAndFnv1,ComputeHash64_NonEmpty_EqualsIndependentImplementation) validate these code paths against independent reference implementations, so this looks robust.If you ever need to regenerate known-answer test vectors, you can temporarily expose these methods or port the reference implementations from the test file into a small console utility.
209-248:ToUInt32LittleEndianandMultiplyMod32correctly implement word-wise modular arithmetic
ToUInt32LittleEndian:
- Interprets
BigIntegervalues viaToByteArray()(little-endian, two’s complement).- Zero-extends when fewer bytes than
unitCount * 4.- Extracts exactly
unitCount32-bit little-endian words, effectively taking the value modulo 2^(32 * unitCount).
MultiplyMod32:
- Performs a standard schoolbook multiplication of two
uintword arrays, accumulating intotmpwith a 32-bit word size.- Ignores overflow beyond
Lwords, which is exactly the desired modulo-2^(32L) behavior for the FNV state.- Skips work for zero words and reuses a caller-provided scratch buffer to avoid repeated allocations.
These helpers line up with the independent multi-word reference implementation used in the tests and look correct for the supported bit sizes (up to 1024 bits).
If you ever need to confirm behavior for edge primes/offset bases, you could compare results against a pure
BigInteger-based implementation similar toIndependentFNVMultiWordacross randomly generated inputs.
24-46: Bit-size validation confirmed—all instantiations use supported sizesVerification of all callers in the codebase confirms that only supported bit sizes {32, 64, 128} are instantiated, plus a single test at line 26 that deliberately uses unsupported size 16 to verify the exception is thrown. The constructor's switch-based validation is properly enforced and correctly restricts usage. Multi-word initialization is well-scoped and correct.
71-78: Code is correct, no changes neededThe constructor validates that
Bitscan only be one of {32, 64, 128, 256, 512, 1024}, enforced via a switch statement that throwsArgumentOutOfRangeExceptionfor invalid values. The dispatcher inComputeHashcorrectly routes all six supported bit-widths through their appropriate paths, and the final throw is indeed unreachable in normal operation, making it purely defensive.test/Cuemon.Core.Tests/Security/FowlerNollVoHashTest.cs (1)
14-47: Test implementation and assertions are correctAll three assertions in the review comment have been verified against the production code at
src/Cuemon.Core/Security/FowlerNollVoHash.cs:
- Bit-size validation throws
ArgumentOutOfRangeExceptionfor unsupported sizes (constructor lines 24–34)- Properties
Bits,Prime, andOffsetBasisare correctly assigned in the constructor (lines 36–38)ComputeHash(byte[])validates null input withValidator.ThrowIfNull(input)(line 73), which throwsArgumentNullExceptionThe test subclass and all three test methods accurately reflect the production behavior.
src/Cuemon.Core/DateSpan.cs (3)
17-20: FNV-based hashing and calendar ID equality look coherentThe introduction of
_calendarIdwith explicit mappings for the built-in calendars and a fallback based on the calendar type, combined with the FNV-64-basedGetHashCodethat mixes_upper.Ticks,_lower.Ticks, and_calendarId, is consistent with the updatedEquals(DateSpan other)implementation and the new tests. This maintains the required invariant: if twoDateSpaninstances are equal, they will always share the same hash code, and different calendar types are distinguishable in hashing while instances of the same type share the same identity.Also applies to: 52-70, 225-240, 261-265
121-132: Average days-per-year helper is logically sound
CalculateAverageDaysPerYearcorrectly aggregatesGetDaysInYearfor each year fromlower.Yearup to but excludingupper.Yearand averages over the computedyears, with theyears == 0case falling back to the single-year day count. This matches the intended use inTotalYearsand preserves existing leap-year behavior.
315-320: Parse overload XML comment clarification is correctThe updated XML comment for the
cultureparameter accurately reflects that theCalendaris resolved from the suppliedCultureInfo, matching the implementation (culture.Calendar) and improving API documentation clarity.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
=======================================
Coverage ? 80.37%
=======================================
Files ? 595
Lines ? 18485
Branches ? 1895
=======================================
Hits ? 14857
Misses ? 3562
Partials ? 66 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.DateSpanBenchmark-report-github.md (1)
1-41: Generated benchmark report looks good.This is a generated BenchmarkDotNet artifact documenting DateSpan performance metrics across .NET 9.0 and 10.0. The results provide valuable insights into the performance characteristics of the refactored DateSpan implementation.
The markdown linting hints (missing language specifier on line 1 and table spacing on line 14) are cosmetic. If you'd like to address them for consistency:
-``` +``` textand add a blank line before line 14. These are optional improvements that don't affect the report's utility.
tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.Security.FowlerNollVoHashBenchmark-report-github.md (1)
1-86: Comprehensive FNV hash benchmark report.This generated BenchmarkDotNet report documents the performance characteristics of the enhanced Fowler–Noll–Vo hash implementation across multiple bit sizes (32–1024) and algorithms (Fnv1, Fnv1a). The metrics provide valuable baseline data for the new multi-word arithmetic implementation.
Similar to the DateSpan report, the markdown linting hints are cosmetic and optional to address.
tooling/Cuemon.Core.Benchmarks/Security/FowlerNollVoHashBenchmark.cs (1)
1-44: Well-designed FNV hash benchmark setup.The benchmark class properly covers both algorithm variants (Fnv1, Fnv1a) across all supported bit widths. The GlobalSetup creates realistic payload sizes for comprehensive performance measurement.
Lines 22-23 contain extra blank lines that could be removed for consistency:
private FowlerNollVo1024 _fnv1024 = null!; - - [GlobalSetup]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Cuemon.sln(4 hunks)Directory.Build.props(5 hunks)Directory.Packages.props(1 hunks)src/Cuemon.Core/Generate.cs(2 hunks)test/Cuemon.Core.Tests/DateSpanTest.cs(2 hunks)test/Cuemon.Core.Tests/GenerateTest.cs(4 hunks)tooling/Cuemon.Core.Benchmarks/.support/Program.cs(1 hunks)tooling/Cuemon.Core.Benchmarks/Cuemon.Core.Benchmarks.csproj(1 hunks)tooling/Cuemon.Core.Benchmarks/DateSpanBenchmark.cs(1 hunks)tooling/Cuemon.Core.Benchmarks/GenerateBenchmark.cs(1 hunks)tooling/Cuemon.Core.Benchmarks/Security/FowlerNollVoHashBenchmark.cs(1 hunks)tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.DateSpanBenchmark-report-github.md(1 hunks)tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.GenerateBenchmark-report-github.md(1 hunks)tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.Security.FowlerNollVoHashBenchmark-report-github.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/Cuemon.Core.Tests/GenerateTest.cs (1)
src/Cuemon.Core/Generate.cs (2)
Generate(18-219)ObjectPortrayal(46-70)
tooling/Cuemon.Core.Benchmarks/DateSpanBenchmark.cs (1)
src/Cuemon.Core/DateSpan.cs (1)
GetWeeks(200-214)
test/Cuemon.Core.Tests/DateSpanTest.cs (1)
src/Cuemon.Core/DateSpan.cs (9)
DateSpan(25-27)DateSpan(34-36)DateSpan(44-100)DateSpan(294-297)DateSpan(305-308)DateSpan(317-320)GetHashCode(223-241)Equals(250-254)Equals(261-265)
tooling/Cuemon.Core.Benchmarks/GenerateBenchmark.cs (1)
src/Cuemon.Core/Generate.cs (3)
Generate(18-219)FixedString(132-135)ObjectPortrayal(46-70)
🪛 markdownlint-cli2 (0.18.1)
tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.Security.FowlerNollVoHashBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.DateSpanBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ 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). (3)
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (20)
Cuemon.sln (1)
3-4: Verify Visual Studio version string format.Line 4 shows
VisualStudioVersion = 18.0.11217.181 d18.0which appears to have unusual formatting with a space and "d18.0" suffix. Confirm this is the correct format for VS2022 solution files.src/Cuemon.Core/Generate.cs (2)
79-89: LGTM! Proper null validation added.The addition of
Validator.ThrowIfNull(generator)with corresponding XML documentation is a good defensive programming practice that prevents potential null reference exceptions.
162-178: Simplified RandomString implementation removes parallelization.The refactored implementation replaces the previous concurrent approach with a straightforward sequential loop. This is clearer and easier to maintain. The addition of the length guard (
if (length <= 0) { return string.Empty; }) properly handles edge cases.However, the removal of parallelization may have performance implications for large strings. Based on the benchmark results in the PR (showing improved performance on .NET 10), this appears to be an intentional optimization.
Note: The benchmark report shows this change improves performance, particularly on .NET 10 where RandomString (256 chars) went from ~7,353ns to ~4,668ns.
test/Cuemon.Core.Tests/GenerateTest.cs (4)
15-55: Excellent test coverage for RangeOf.The tests comprehensively validate:
- Correct sequence generation with transformation logic
- Empty sequence for count=0 without invoking the generator
- Index parameter correctness and ordering
- Error handling for negative counts
58-102: Thorough validation of RandomString behavior.These tests properly verify:
- Output length matches input
- Characters are drawn only from allowed sets
- Zero-length input returns empty string
- Null and empty value arrays throw appropriate exceptions
The test at lines 79-90 using custom values with distinct character validation is particularly good for ensuring the implementation respects the input character sets.
127-197: Comprehensive hash code testing.The tests validate critical hash code properties:
- Consistency between params and enumerable overloads
- Determinism (same input → same hash)
- Order sensitivity (different order → different hash)
- Empty sequence handling across all overloads
This provides strong guarantees for the FNV-1a hash implementation used by these methods.
199-290: ObjectPortrayal tests validate complex behavior.The tests effectively cover:
- Null handling
- Boolean lowercase conversion
- ToString override detection and bypass
- Property enumeration including write-only properties
The helper classes (WithToStringOverride, NoOverride, CallsObjectPortrayalFromToString) appropriately test different scenarios including the recursive call protection via
BypassOverrideCheck.test/Cuemon.Core.Tests/DateSpanTest.cs (1)
14-66: Excellent hash code test coverage for DateSpan.These tests validate the critical properties of the new hash code implementation:
- Stability: Multiple calls to GetHashCode return the same value (lines 14-25)
- Equality contract: Equal instances produce equal hash codes (lines 27-35)
- Sensitivity: Changes in data (boundaries) affect the hash (lines 37-44)
- Calendar awareness: Same calendar types produce equal hashes, different types produce different hashes (lines 46-66)
This aligns well with the DateSpan.GetHashCode implementation that uses FNV-1a hashing with
_calendarIdas a discriminator.Directory.Build.props (3)
4-4: LGTM! Consistent project type detection pattern.The
IsBenchmarkProjectproperty follows the same pattern asIsTestProject, using the project name suffix to determine the project type. This is a clean and maintainable approach.
17-17: Proper conditional logic for benchmark project segregation.The conditions correctly:
- Line 17, 50: Exclude benchmark projects from production package settings using AND logic
- Line 64: Include benchmark projects in test-like settings (no packaging, disabled analyzers) using OR logic
This ensures benchmark projects are treated appropriately—not packaged as libraries but configured similarly to test projects.
Also applies to: 50-50, 64-64
101-108: LGTM! Benchmark project configuration is appropriate.The configuration:
- Sets
TargetFrameworkstonet10.0;net9.0;net48for cross-framework benchmarking- Includes BenchmarkDotNet and platform-specific diagnostics package
This aligns with standard BenchmarkDotNet project setup and the benchmark reports in the PR.
tooling/Cuemon.Core.Benchmarks/Cuemon.Core.Benchmarks.csproj (1)
1-19: LGTM! Standard benchmark project structure.The project configuration is clean and follows best practices:
- Uses SDK-style project format
- Maintains consistent namespace with the main project (
RootNamespace = Cuemon)- References the target library (Cuemon.Core) for benchmarking
- Organizes entry point in
.support\Program.cs- Includes folder for benchmark results
This aligns with the benchmark dependencies configured in
Directory.Build.propsandDirectory.Packages.props.tooling/Cuemon.Core.Benchmarks/reports/results/Cuemon.GenerateBenchmark-report-github.md (1)
1-104: Benchmark results validate performance improvements.This generated report documents the performance characteristics of the refactored Generate methods. Key observations:
- RandomString shows significant speedup on .NET 10 (e.g., 256-char strings: ~7.4μs → ~4.7μs)
- RangeOf shows modest improvements across frameworks
- HashCode operations maintain consistent performance
These results support the PR's objective of achieving AI-assisted performance boosts while maintaining correctness (validated by the comprehensive test suite).
tooling/Cuemon.Core.Benchmarks/DateSpanBenchmark.cs (2)
1-47: Well-structured benchmark setup.The class configuration and GlobalSetup method are properly implemented. The use of ISO 8601 sortable format with InvariantCulture ensures consistent parsing behavior across benchmarks, and the variety of test data (short/medium/long spans) provides good coverage.
49-94: Comprehensive benchmark coverage of DateSpan operations.The benchmark methods cover key scenarios including construction, parsing, formatting, week calculation, hashing, and equality checks. The equality benchmarks (lines 86-93) include the overhead of constructing new DateSpan instances, which represents realistic usage patterns rather than isolated comparison performance.
tooling/Cuemon.Core.Benchmarks/.support/Program.cs (2)
26-50: Solid BenchmarkDotNet configuration.The configuration follows best practices from the dotnet/performance repository with appropriate customization. The job settings balance execution time and reliability, and the export/logging setup ensures comprehensive results documentation.
The artifacts path computation on line 29 uses relative directory navigation (
"..", "..", ".."), which assumes the assembly location is three directories deep from the project root. Verify that this works correctly across different build configurations (Debug/Release) and deployment scenarios.
52-58: Clean benchmark execution logic.The default argument handling provides sensible defaults (all benchmarks, .NET 9.0 and 10.0 runtimes) while preserving command-line override capability. The BenchmarkSwitcher approach enables flexible benchmark discovery and execution.
tooling/Cuemon.Core.Benchmarks/Security/FowlerNollVoHashBenchmark.cs (1)
46-92: Comprehensive benchmark coverage of FNV hash operations.The benchmark methods systematically cover all combinations of hash bit widths (32, 64, 128, 256, 512, 1024) and payload sizes. The consistent structure and clear naming make the results easy to interpret.
tooling/Cuemon.Core.Benchmarks/GenerateBenchmark.cs (2)
1-36: Effective benchmark parameterization and setup.The parameterized Count values (8, 256, 4096) enable performance measurement across different scales. The GlobalSetup prepares realistic test data including a sample object, string values, and convertibles collection for comprehensive coverage of Generate utility methods.
38-105: Thorough coverage of Generate utility methods.The benchmark methods effectively cover the key Generate API surface including RangeOf, RandomNumber, string generation, ObjectPortrayal, and hash code computation. The RangeOf benchmark correctly uses a sink to prevent the compiler from optimizing away the enumeration. The Sample class provides appropriate test data for ObjectPortrayal benchmarking.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Cuemon.Security.Cryptography/SecureHashAlgorithm512.cs (1)
22-25: Clarify XML docs for now-optionalsetupparameterMaking
setupoptional is a nice usability improvement and should be backward-compatible. Consider updating the<param name="setup">doc (and possibly the summary) to state that the parameter is optional, may benull, and what behavior occurs when it is omitted (e.g., default conversion options / no custom configuration).src/Cuemon.Security.Cryptography/KeyedCryptoHash.cs (1)
29-34: Consider removing the redundant override.The
ComputeHashimplementation inKeyedCryptoHash<TAlgorithm>is identical to the base classUnkeyedCryptoHash<TAlgorithm>implementation. Since the base class already provides this exact functionality, the override appears unnecessary and adds maintenance burden.If there's no specific reason to maintain this override (e.g., future extensibility plans or documentation purposes), consider removing it:
- /// <summary> - /// Computes the hash value for the specified <see cref="T:byte[]" />. - /// </summary> - /// <param name="input">The <see cref="T:byte[]" /> to compute the hash code for.</param> - /// <returns>A <see cref="HashResult" /> containing the computed hash code of the specified <paramref name="input" />.</returns> - public override HashResult ComputeHash(byte[] input) - { - Validator.ThrowIfNull(input); - using var h = Initializer(); - return new HashResult(h.ComputeHash(input)); - }Directory.Build.props (2)
80-82: WarningLevel=0 for tests outside VS may hide real compiler issuesThe new test block:
<PropertyGroup Condition="'$(IsTestProject)' == 'true' AND '$(BuildingInsideVisualStudio)' != 'true'"> <WarningLevel>0</WarningLevel> </PropertyGroup>completely disables C# compiler warnings for all test projects on non‑VS builds (e.g.,
dotnet build, CI). Given you already relax analyzers for tests, this is a pretty strong setting and can mask genuine problems (e.g., obsolete API usage, nullable issues) that only show up in tests.If the intent is just to reduce noise in CI, you might consider a narrower approach (tweaking
NoWarnor specific analyzer rules) rather than globally forcingWarningLevelto 0.Please verify this is intentional for your pipelines; otherwise a follow‑up PR could dial this back to a more targeted suppression strategy.
4-4: Benchmark project wiring looks good; consider gating Windows diagnostics packageThe
IsBenchmarkProjectflag and associated Property/ItemGroups are consistent with MSBuild string property functions and give benchmarks their own TFM set and package references:<IsBenchmarkProject>$(MSBuildProjectName.EndsWith('Benchmarks'))</IsBenchmarkProject> <PropertyGroup Condition="'$(IsBenchmarkProject)' == 'true'"> <TargetFrameworks>net10.0;net9.0;netstandard2.0</TargetFrameworks> </PropertyGroup> <ItemGroup Condition="'$(IsBenchmarkProject)' == 'true'"> <PackageReference Include="BenchmarkDotNet" /> <PackageReference Include="BenchmarkDotNet.Diagnostics.Windows" /> </ItemGroup>One minor refinement:
BenchmarkDotNet.Diagnostics.Windowsis Windows‑specific. To avoid dragging this into non‑Windows environments where it can’t run, you could gate it onIsWindows:- <ItemGroup Condition="'$(IsBenchmarkProject)' == 'true'"> - <PackageReference Include="BenchmarkDotNet" /> - <PackageReference Include="BenchmarkDotNet.Diagnostics.Windows" /> - </ItemGroup> + <ItemGroup Condition="'$(IsBenchmarkProject)' == 'true'"> + <PackageReference Include="BenchmarkDotNet" /> + </ItemGroup> + + <ItemGroup Condition="'$(IsBenchmarkProject)' == 'true' AND '$(IsWindows)' == 'true'"> + <PackageReference Include="BenchmarkDotNet.Diagnostics.Windows" /> + </ItemGroup>This keeps benchmarks cross‑platform while still enabling the Windows diagnostics when relevant.
Please confirm that all your benchmark projects either run only on Windows or that you’re okay pulling in the Windows‑only diagnostics package on other platforms as a no‑op dependency.
Also applies to: 112-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Directory.Build.props(5 hunks)src/Cuemon.Core/DelimitedString.cs(4 hunks)src/Cuemon.Core/Patterns.cs(2 hunks)src/Cuemon.Security.Cryptography/AesCryptor.cs(2 hunks)src/Cuemon.Security.Cryptography/KeyedCryptoHash.cs(1 hunks)src/Cuemon.Security.Cryptography/SecureHashAlgorithm512.cs(1 hunks)src/Cuemon.Security.Cryptography/UnkeyedCryptoHash.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Cuemon.Security.Cryptography/AesCryptor.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/Cuemon.Security.Cryptography/KeyedCryptoHash.cs (2)
src/Cuemon.Core/Validator.cs (1)
Validator(15-1150)src/Cuemon.Security.Cryptography/UnkeyedCryptoHash.cs (1)
HashResult(39-44)
src/Cuemon.Security.Cryptography/UnkeyedCryptoHash.cs (2)
src/Cuemon.Core/Validator.cs (1)
Validator(15-1150)src/Cuemon.Security.Cryptography/KeyedCryptoHash.cs (1)
HashResult(29-34)
⏰ 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). (3)
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
src/Cuemon.Core/Patterns.cs (2)
103-108: LGTM! Performance improvement by avoiding reflection.Replacing
Activator.CreateInstance<T>()with direct constructornew T()is more performant and equally correct given thenew()constraint.
119-126: LGTM! Performance improvement by avoiding reflection.Replacing
Activator.CreateInstance<TOptions>()with direct constructornew TOptions()is more performant and equally correct given thenew()constraint.src/Cuemon.Security.Cryptography/KeyedCryptoHash.cs (1)
31-33: LGTM! Modern syntax and explicit validation improve the code.The addition of explicit null validation and the migration to using declarations are both solid improvements that enhance readability and follow modern C# conventions.
src/Cuemon.Security.Cryptography/UnkeyedCryptoHash.cs (1)
41-43: LGTM! Consistent improvements to the base implementation.The explicit null validation and using declaration mirror the pattern applied in the derived class, providing a consistent approach to resource management and input validation across the cryptographic hashing components.
Directory.Build.props (1)
5-6: Based on my verification, I can now provide the final assessment:The review comment's technical analysis is incorrect; the current code actually works as intended.
The review claims that nested property expansion
$($(MSBuildThisFileDirectory.ToLower())src)doesn't work and that the expressions only match literal strings"src"and"tooling". However, this misunderstands how MSBuild property functions operate:
- MSBuild does expand property references in function arguments at evaluation time
- When evaluating
StartsWith('$(MSBuildThisFileDirectory.ToLower())src'), the inner$(MSBuildThisFileDirectory.ToLower())is expanded to the actual directory path first- For a project at
/repo/src/Cuemon.Core/, this becomesStartsWith('/repo/src'), which correctly matches- Evidence: The repository has successful builds, passing tests, and published packages (V10.0.0 launched successfully), which would fail if
IsSourceProjectnever evaluated to true for src projectsHowever, the suggestion to use
StringComparison.OrdinalIgnoreCasefor clarity and efficiency is still potentially valid, though:
- It's not required to fix an actual bug (the current code works)
- The suggested syntax using
StringComparisonenum directly may not be compatible with all MSBuild versions or contexts- No existing uses of
StringComparisonin property functions appear in the codebaseThe review comment conflates a style/efficiency suggestion with a critical bug that doesn't exist.
Likely an incorrect or invalid review comment.
src/Cuemon.Core/DelimitedString.cs (5)
14-14: LGTM: Improved cache key type safety.Changing from a string-based key to a tuple key
(string Delimiter, string Qualifier)improves type safety and better expresses the cache key's intent.
30-43: LGTM: Cleaner delimiter handling.The refactor to handle the first element separately and then prepend delimiters for subsequent items is a cleaner pattern that avoids the need for trailing delimiter trimming. The early return for empty sources is also a good optimization.
67-67: LGTM: Good optimization for common case.Adding a fast path for single-character delimiters and qualifiers should significantly improve performance for typical CSV scenarios.
69-72: I'll help you verify the review comment aboutRegex.Escapebehavior with qualifiers in character classes. Let me start by exploring the codebase to understand howDelimitedString.Splitis tested and what qualifiers are actually used.
<function_calls>
#!/bin/bashSearch for tests of DelimitedString.Split with various qualifiers
rg -n -C 5 'DelimitedString.*Split|DelimitedStringOptions.*Qualifier' --type=cs -g 'Test.cs' | head -100
#!/bin/bash # Find test files related to DelimitedString fd -e cs -g '*Test*.cs' | xargs rg -l 'DelimitedString' | head -20 #!/bin/bash # Look at the DelimitedString implementation to understand the full context find . -name 'DelimitedString.cs' -type f
</function_calls>
84-117: RFC-4180 conformance claim requires clarification regarding doubled qualifier interpretation.The implementation at line 54 claims RFC-4180 conformance, but RFC-4180 section 2.7 specifies that doubled qualifiers (
"") within quoted fields should represent a single literal qualifier. TheSplitSingleCharCsvmethod togglesinQuoteson every qualifier without interpreting doubled qualifiers as escape sequences.However, the test
Split_ShouldSplitPreservingQualifierOfTypeDoubleQuotevalidates that the implementation correctly handles input containing doubled qualifiers through a round-trip test (split + rejoin = original). This works because raw field data is preserved, not because doubled qualifiers are interpreted as escaped quotes.The question is: does RFC-4180 conformance in this library mean (1) preserving raw input format (currently working), or (2) interpreting and unescaping doubled qualifiers (not currently implemented)? The existing test suggests the former is intentional, but the RFC-4180 reference should be clarified in documentation.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
test/Cuemon.Security.Cryptography.Tests/UnkeyedCryptoHashTest.cs (2)
30-36: Ctor guard test is precise; consider extending coverage if base validates more argsThe ctor test correctly asserts both the exception type and
ParamNamefor a null initializer, which tightly couples to the guard you care about. IfUnkeyedCryptoHashalso guards other parameters (e.g., algorithm name), you might later add a similar negative test for those to keep coverage symmetric.
38-55: Hash computation tests correctly cross-check against framework SHA256Both tests validate
HasValueand compare againstSHA256.ComputeHashfor non-empty and empty input, which is exactly what you want to ensure the wrapper stays in lockstep with the platform implementation. If you add more unkeyed-hash tests in future, consider extracting the “compute expected via SHA256 and compareGetBytes()” logic into a small helper to avoid duplication, but it’s not necessary for the current size.Also applies to: 57-73
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm512Test.cs (1)
1-68: LGTM! Comprehensive HMAC-SHA512 test coverage.The test suite correctly validates HmacSecureHashAlgorithm512 against
System.Security.Cryptography.HMACSHA512with appropriate scenario coverage.Optional: Consider reducing test code duplication.
The HMAC test files (256, 384, 512) share nearly identical structure. Consider extracting a common test base class or using theory/inline data to reduce duplication, though this is a minor maintainability suggestion and not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Cuemon.Security.Cryptography/AesCryptor.cs(3 hunks)test/Cuemon.Core.Tests/Security/HashResultTest.cs(1 hunks)test/Cuemon.Core.Tests/Security/HashTest.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/AesCryptorTest.cs(2 hunks)test/Cuemon.Security.Cryptography.Tests/HmacMessageDigest5Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm1Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm256Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm384Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm512Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/KeyedCryptoHashTest.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/MessageDigest5Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/SHA512256Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm1Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm256Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm384Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm512Test.cs(1 hunks)test/Cuemon.Security.Cryptography.Tests/UnkeyedCryptoHashTest.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
test/Cuemon.Security.Cryptography.Tests/AesCryptorTest.cs (1)
src/Cuemon.Security.Cryptography/AesCryptor.cs (4)
GenerateInitializationVector(109-116)GenerateKey(123-131)Encrypt(68-71)Decrypt(79-82)
test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm512Test.cs (1)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)
test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm256Test.cs (1)
test/Cuemon.Core.Tests/Security/HashTest.cs (3)
Fact(34-40)Fact(42-49)Fact(51-58)
test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm1Test.cs (2)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)test/Cuemon.Core.Tests/Security/HashTest.cs (3)
Fact(34-40)Fact(42-49)Fact(51-58)
test/Cuemon.Core.Tests/Security/HashTest.cs (1)
src/Cuemon.Core/Convertible.cs (1)
Convertible(14-487)
test/Cuemon.Security.Cryptography.Tests/KeyedCryptoHashTest.cs (2)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)test/Cuemon.Core.Tests/Security/HashTest.cs (3)
Fact(34-40)Fact(42-49)Fact(51-58)
test/Cuemon.Security.Cryptography.Tests/HmacMessageDigest5Test.cs (1)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm384Test.cs (2)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)test/Cuemon.Core.Tests/Security/HashTest.cs (3)
Fact(34-40)Fact(42-49)Fact(51-58)
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm256Test.cs (2)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)test/Cuemon.Core.Tests/Security/HashTest.cs (3)
Fact(34-40)Fact(42-49)Fact(51-58)
test/Cuemon.Security.Cryptography.Tests/UnkeyedCryptoHashTest.cs (1)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (1)
test/Cuemon.Core.Tests/Security/HashTest.cs (18)
Fact(34-40)Fact(42-49)Fact(51-58)Fact(60-67)Fact(69-76)Fact(78-85)Fact(87-94)Fact(96-103)Fact(105-112)Fact(114-121)Fact(123-130)Fact(132-139)Fact(141-148)Fact(150-157)Fact(159-166)Fact(168-175)HashResult(20-23)HashResult(235-239)
test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm384Test.cs (2)
test/Cuemon.Core.Tests/Security/HashResultTest.cs (13)
Fact(13-19)Fact(21-32)Fact(34-47)Fact(49-57)Fact(59-66)Fact(68-78)Fact(80-90)Fact(92-98)Fact(100-105)Fact(107-112)Fact(114-122)Fact(124-132)Fact(134-144)test/Cuemon.Core.Tests/Security/HashTest.cs (3)
Fact(34-40)Fact(42-49)Fact(51-58)
⏰ 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). (3)
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (42)
test/Cuemon.Security.Cryptography.Tests/UnkeyedCryptoHashTest.cs (1)
15-28: Well-scoped test-only hash subclassesThe two sealed nested types cleanly separate the “happy path” (
Sha256TestHash) from the guard-path (NullInitializerHash), making intent obvious and keeping production code untouched. This is a solid pattern for exercising base-class behavior from tests.test/Cuemon.Security.Cryptography.Tests/MessageDigest5Test.cs (1)
1-70: LGTM! Comprehensive test coverage for MessageDigest5.The test suite appropriately validates the MD5 implementation against
System.Security.Cryptography.MD5, covers edge cases (empty input, null inputs), and follows consistent testing patterns seen across the codebase.test/Cuemon.Core.Tests/Security/HashResultTest.cs (1)
1-146: LGTM! Thorough test coverage for HashResult.The test suite comprehensively validates HashResult behavior including value semantics, independent byte array copying, multiple string representations, equality/hashing semantics, and converter patterns. The reference-based equality behavior is clearly documented in the tests.
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm256Test.cs (1)
1-68: LGTM! Comprehensive HMAC-SHA256 test coverage.The test suite correctly validates HmacSecureHashAlgorithm256 against
System.Security.Cryptography.HMACSHA256and covers standard test scenarios (non-empty input, empty input, null secret, null input exception).test/Cuemon.Security.Cryptography.Tests/KeyedCryptoHashTest.cs (1)
1-75: LGTM! KeyedCryptoHash test coverage is solid.The test suite validates
KeyedCryptoHash<T>using HMAC-SHA256 and HMAC-MD5 implementations, correctly comparing against standard cryptographic libraries and covering null input handling.test/Cuemon.Core.Tests/Security/HashTest.cs (1)
1-343: LGTM! Excellent comprehensive test coverage for Hash abstraction.The test suite thoroughly validates the Hash abstraction across all supported types and scenarios. The PassthroughHash and TestHash test implementations are well-designed to isolate and verify specific behaviors. Coverage includes:
- All primitive type overloads
- String encoding configurations
- Enum handling
- Parameter and enumerable aggregation
- Stream processing
- Endianness configuration
- Options initialization
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm384Test.cs (1)
1-68: LGTM! Comprehensive HMAC-SHA384 test coverage.The test suite correctly validates HmacSecureHashAlgorithm384 against
System.Security.Cryptography.HMACSHA384with appropriate scenario coverage.test/Cuemon.Security.Cryptography.Tests/HmacMessageDigest5Test.cs (1)
1-68: LGTM! Comprehensive HMAC-MD5 test coverage.The test suite correctly validates HmacMessageDigest5 against
System.Security.Cryptography.HMACMD5with appropriate scenario coverage. While MD5 is cryptographically broken for security purposes, maintaining test coverage for legacy compatibility is acceptable.test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm1Test.cs (4)
1-12: LGTM! Clean test class structure.The imports, namespace, class structure, and constructor are properly organized and follow standard xUnit patterns.
14-31: LGTM! Well-structured test with proper resource management.The test correctly validates HMAC-SHA1 computation against the standard library reference implementation and properly disposes of the HMACSHA1 instance.
33-50: LGTM! Good edge case coverage.Testing with an empty array ensures the implementation handles edge cases correctly. The test follows the same solid pattern as the previous test.
52-58: LGTM! Correct validation of null-secret handling.The test properly verifies that the constructor accepts a null secret without throwing an exception using Record.Exception.
test/Cuemon.Security.Cryptography.Tests/SHA512256Test.cs (6)
1-11: LGTM!Standard test class setup with appropriate dependencies.
13-18: LGTM!Constructor test correctly verifies the hash size and properly disposes the instance.
20-36: LGTM!The reflection-based test correctly verifies big-endian byte writing at a specific offset.
38-58: LGTM!Good test coverage for bit rotation with both edge cases and arbitrary rotations.
156-171: LGTM!Excellent test that verifies both the output length and deterministic behavior of the hash function with proper resource management.
173-184: LGTM!Good edge-case test for empty input with proper resource management.
test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm512Test.cs (4)
14-18: BitSize test correctly validates 512 bitsThe test targets
SecureHashAlgorithm512.BitSizeand the expected value matches the algorithm size.
20-27: Constructor options test is exercising the right behaviorThe ctor-based setup correctly configures
ByteOrdertoBigEndianand asserts againstsut.Options.
29-45: SHA‑512 hash test for non‑empty input is soundThe test compares
ComputeHash("hello")againstSHA512.Create().ComputeHash, which is the right oracle and ensuresHasValueis true.
47-63: SHA‑512 hash test for empty input is correctEmpty array handling is validated against
SHA512.Create().ComputeHash(Array.Empty<byte>()), which gives good coverage of the edge case.test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm384Test.cs (4)
14-18: BitSize test correctly validates 384 bitsThe test asserts
SecureHashAlgorithm384.BitSize == 384, matching the algorithm’s width.
20-27: Constructor options test correctly wires ByteOrderThe setup delegate configures
ByteOrdertoBigEndianand verifiessut.Optionsaccordingly.
29-45: SHA‑384 hash test for non‑empty input is well‑formedThe result of
ComputeHash("hello")is compared toSHA384.Create().ComputeHash, giving a solid reference check.
47-63: SHA‑384 hash test for empty input covers the edge caseEmpty input behavior is validated against the framework’s SHA‑384 implementation, mirroring the non‑empty case.
test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm256Test.cs (3)
14-18: BitSize test correctly validates 256 bitsThe assertion on
SecureHashAlgorithm256.BitSizematching 256 is appropriate.
29-45: SHA‑256 hash test for non‑empty input is correctThe test compares
ComputeHash("hello")againstSHA256.Create().ComputeHash, providing a reliable reference.
47-63: SHA‑256 hash test for empty input is appropriateEmpty array handling is verified against
SHA256.Create().ComputeHash(Array.Empty<byte>()), giving good coverage.test/Cuemon.Security.Cryptography.Tests/SecureHashAlgorithm1Test.cs (4)
14-18: BitSize test correctly validates 160 bitsThe test asserts
SecureHashAlgorithm1.BitSize == 160, which aligns with SHA‑1.
20-27: Constructor options test validates ByteOrder wiringUsing the setup delegate to set
ByteOrdertoBigEndianand asserting viasut.Optionsis correct.
29-45: SHA‑1 hash test for non‑empty input is soundThe algorithm output for
"hello"is verified againstSHA1.Create().ComputeHash, ensuring correctness.
47-63: SHA‑1 hash test for empty input correctly covers the edge caseEmpty array hashing is validated against the framework SHA‑1 implementation, mirroring other tests.
src/Cuemon.Security.Cryptography/AesCryptor.cs (1)
96-100: Excellent refactoring to direct transform API.The migration from a streaming approach (MemoryStream/CryptoStream) to the direct
TransformFinalBlockpattern simplifies the code and eliminates unnecessary overhead for in-memory operations. This is the idiomatic .NET approach for encrypting/decrypting byte arrays that are already loaded in memory.test/Cuemon.Security.Cryptography.Tests/AesCryptorTest.cs (8)
38-50: Comprehensive default constructor validation.The test correctly validates that the default constructor produces cryptographically valid key and IV lengths (256-bit key for AES-256, 128-bit IV for block size). The inline comments and use of
Convertible.BitsPerByteenhance readability.
52-63: Good validation of reference semantics.Using
Assert.Sameto verify that the constructor stores the exact key and IV references (rather than defensive copies) is the right approach for confirming the expected reference semantics.
65-77: Proper null-argument validation coverage.These tests correctly validate that the constructor throws
ArgumentNullExceptionfor null key or IV parameters, ensuring proper precondition enforcement.
79-98: Thorough validation of size constraints.These tests properly validate that invalid key or IV sizes throw
CryptographicExceptionwith appropriate error messages, ensuring the constructor enforces AES size requirements (128/192/256-bit keys, 128-bit IV).
100-118: Essential end-to-end encryption validation.This test provides critical validation of the core encrypt/decrypt functionality, confirming that encryption produces different output and decryption successfully recovers the original plaintext.
120-137: Good coverage of options configuration path.Testing the options delegate overload with explicit parameters (even though they match defaults) ensures the options configuration mechanism works correctly. The inline comment clearly explains the test's intent.
139-152: Appropriate validation of IV generation.The test correctly validates IV length and typical uniqueness between successive calls. The comment acknowledging the small collision probability demonstrates appropriate awareness of the probabilistic nature of randomness testing.
154-174: Comprehensive key generation validation.This test thoroughly validates the
GenerateKeymethod across multiple scenarios: default AES-256, explicit AES-128 size, and customRandomStringProvider. The coverage ensures both default behavior and configurability work correctly.
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm256Test.cs
Show resolved
Hide resolved
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm384Test.cs
Show resolved
Hide resolved
test/Cuemon.Security.Cryptography.Tests/HmacSecureHashAlgorithm512Test.cs
Show resolved
Hide resolved
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/Cuemon.Core.Tests/Security/FowlerNollVo128Test.cs (1)
42-81: Duplicated helper method.This
IndependentFNVMultiWordimplementation is identical to those inFowlerNollVo256Test,FowlerNollVo512Test, andFowlerNollVo1024Test. See the comment onFowlerNollVo1024Testfor the refactoring suggestion.test/Cuemon.Core.Tests/Security/FowlerNollVo512Test.cs (1)
38-77: Duplicated helper method.Same
IndependentFNVMultiWordduplication as noted in other multi-word FNV test files. See the comment onFowlerNollVo1024Testfor the refactoring suggestion.
🧹 Nitpick comments (8)
test/Cuemon.Core.Tests/Security/FowlerNollVo64Test.cs (1)
19-37: Consider expanding test coverage for consistency with FowlerNollVo32Test.Compared to
FowlerNollVo32Test, this test file is missing:
- FNV-1 algorithm validation (line 33 only sets
Fnv1a)- Little-endian byte order test
- Verification of actual offset basis bytes in the empty hash test (only checks length)
These gaps reduce confidence in the 64-bit implementation's correctness across all configuration options.
test/Cuemon.Core.Tests/Security/FowlerNollVo1024Test.cs (1)
38-77: Extract duplicated helper to shared test utility.
IndependentFNVMultiWordis duplicated verbatim acrossFowlerNollVo128Test,FowlerNollVo256Test,FowlerNollVo512Test, andFowlerNollVo1024Test. Consider extracting it to a shared test utility class (e.g.,FowlerNollVoTestHelper) to reduce maintenance burden and ensure consistency.- private static byte[] IndependentFNVMultiWord(byte[] input, BigInteger prime, BigInteger offsetBasis, short bits, FowlerNollVoOptions options) - { - // ... implementation - } + // Move to: test/Cuemon.Core.Tests/Security/FowlerNollVoTestHelper.cs + // internal static class FowlerNollVoTestHelper + // { + // public static byte[] IndependentFNVMultiWord(...) { ... } + // }test/Cuemon.Core.Tests/Security/FowlerNollVo256Test.cs (2)
12-25: Strengthen empty-input test to actually verify the offset-basis value
ComputeHash_Empty_ReturnsOffsetBasis_LengthIs32Bytescurrently only asserts the length of the hash. The name and FNV semantics suggest we should also verify that the bytes correspond to the offset basis for the configured options.You can tighten this by using the existing independent implementation to compute the expected empty-hash and asserting equality:
[Fact] public void ComputeHash_Empty_ReturnsOffsetBasis_LengthIs32Bytes() { var sut = new FowlerNollVo256(); var result = sut.ComputeHash(Array.Empty<byte>()).GetBytes(); - Assert.Equal(32, result.Length); + Assert.Equal(32, result.Length); + + var expected = IndependentFNVMultiWord(Array.Empty<byte>(), sut.Prime, sut.OffsetBasis, sut.Bits, sut.Options); + Assert.Equal(expected, result); }
38-77: DRY upIndependentFNVMultiWordacross FNV test variantsThis helper is effectively identical to the versions in
FowlerNollVo128TestandFowlerNollVo1024Test(and likely other bit-width tests). Duplicating this BigInteger implementation in each test file makes future fixes or tweaks error-prone.Consider moving
IndependentFNVMultiWordinto a shared test helper (e.g., a static class in the test project) and reusing it across all FNV tests. While you’re there, you could also simplify the copy loop withBuffer.BlockCopyorArray.Copyinstead of assigning each byte individually.src/Cuemon.Core/Security/CyclicRedundancyCheck.cs (1)
12-41: Lookup-table refactor removes allocations and keeps initialization clearThe move to
Lazy<ulong[]>plus an array-returningPolynomialTableInitializerCorenicely avoids the extraList+ToArrayoverhead while preserving the existing table semantics used byCyclicRedundancyCheck32/64and the new tests. The protectedLookupTablenow exposes the backing array directly, which is fine for trusted derived types; if you ever want stronger immutability guarantees, you could later switch the property to anIReadOnlyList<ulong>or return a copy, but that’s not necessary for this change set.Also applies to: 47-47
test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck64Test.cs (1)
1-121: Comprehensive CRC64 coverage; minor test polish and import checkThese tests give good confidence: they validate constructor defaults, both internal primitives via reflection, lookup-table contents against a recomputation, memoization across accesses, and the standard CRC‑64‑ECMA‑182 vector for
"123456789".Two small follow-ups you might consider:
- Use
nameof(CyclicRedundancyCheck64.PolynomialIndexInitializer)/nameof(CyclicRedundancyCheck64.PolynomialSlotCalculator)instead of string literals inGetMethodto make the reflection a bit more refactor‑friendly.- Optionally add a
ComputeHashtest that assertsArgumentNullExceptionis thrown fornullinput, to lock in the newValidator.ThrowIfNull(input)behavior.Also, please confirm that
ITestOutputHelperis available in this project via an existingusingor global using (e.g.,Xunit.Abstractions), since it’s referenced here but not explicitly imported in this file.test/Cuemon.Core.Tests/Security/CyclicRedundancyCheckTest.cs (1)
1-82: Good focused tests for CRC lookup-table generation and memoizationThe
TestCrchelper cleanly isolates the table-generation behavior, and the tests around initial/final values, specific table entries, polynomial sensitivity, and repeated access all line up well with the newLazy<ulong[]>implementation.If you want to refine this further:
- The inline comments already document the arithmetic; you could mirror that style in
TestCrcitself (e.g., brief comment inPolynomialIndexInitializer) to keep behavior obvious where it’s defined.- As with the other test classes, please double‑check that
ITestOutputHelperis in scope via an appropriate using/global using for the project configuration.Overall, this is a solid white‑box test harness around the CRC base class.
test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck32Test.cs (1)
1-113: Thorough CRC32 tests aligned with implementation and standard vectorThis suite nicely exercises constructor defaults, internal index/slot behavior via reflection, the lookup table contents against a recomputation, and the canonical CRC‑32 (IEEE 802.3) value for
"123456789"with input/output reflection enabled.Possible light improvements:
- Replace string literals in
GetMethodwithnameof(CyclicRedundancyCheck32.PolynomialIndexInitializer)/nameof(CyclicRedundancyCheck32.PolynomialSlotCalculator)to make the tests more resilient to refactors.- Optionally add a test that
ComputeHash(null)throwsArgumentNullExceptionto pin down the newValidator.ThrowIfNull(input)behavior.As with the other test files, please verify that
ITestOutputHelperis correctly brought into scope (e.g., viaXunit.Abstractionsor a global using) given it’s referenced in the constructor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Cuemon.Core/Security/CyclicRedundancyCheck.cs(2 hunks)src/Cuemon.Core/Security/CyclicRedundancyCheck32.cs(1 hunks)src/Cuemon.Core/Security/CyclicRedundancyCheck64.cs(1 hunks)test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck32Test.cs(1 hunks)test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck64Test.cs(1 hunks)test/Cuemon.Core.Tests/Security/CyclicRedundancyCheckTest.cs(1 hunks)test/Cuemon.Core.Tests/Security/FowlerNollVo1024Test.cs(1 hunks)test/Cuemon.Core.Tests/Security/FowlerNollVo128Test.cs(1 hunks)test/Cuemon.Core.Tests/Security/FowlerNollVo256Test.cs(1 hunks)test/Cuemon.Core.Tests/Security/FowlerNollVo32Test.cs(1 hunks)test/Cuemon.Core.Tests/Security/FowlerNollVo512Test.cs(1 hunks)test/Cuemon.Core.Tests/Security/FowlerNollVo64Test.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
test/Cuemon.Core.Tests/Security/FowlerNollVo64Test.cs (1)
test/Cuemon.Core.Tests/Security/FowlerNollVo1024Test.cs (1)
Fact(12-17)
test/Cuemon.Core.Tests/Security/FowlerNollVo512Test.cs (1)
src/Cuemon.Core/Convertible.cs (2)
Convertible(14-487)ReverseEndianness(456-469)
test/Cuemon.Core.Tests/Security/FowlerNollVo1024Test.cs (3)
test/Cuemon.Core.Tests/Security/FowlerNollVo128Test.cs (1)
IndependentFNVMultiWord(42-81)test/Cuemon.Core.Tests/Security/FowlerNollVo256Test.cs (1)
IndependentFNVMultiWord(38-77)test/Cuemon.Core.Tests/Security/FowlerNollVo512Test.cs (1)
IndependentFNVMultiWord(38-77)
src/Cuemon.Core/Security/CyclicRedundancyCheck32.cs (1)
src/Cuemon.Core/Validator.cs (1)
Validator(15-1150)
test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck64Test.cs (2)
test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck32Test.cs (5)
Fact(15-21)Fact(23-37)Fact(39-61)Fact(63-95)Fact(97-112)test/Cuemon.Core.Tests/Security/CyclicRedundancyCheckTest.cs (4)
Fact(13-19)Fact(21-33)Fact(35-45)Fact(47-55)
test/Cuemon.Core.Tests/Security/FowlerNollVo128Test.cs (3)
test/Cuemon.Core.Tests/Security/FowlerNollVo1024Test.cs (1)
IndependentFNVMultiWord(38-77)test/Cuemon.Core.Tests/Security/FowlerNollVo256Test.cs (1)
IndependentFNVMultiWord(38-77)src/Cuemon.Core/Convertible.cs (2)
Convertible(14-487)ReverseEndianness(456-469)
test/Cuemon.Core.Tests/Security/CyclicRedundancyCheckTest.cs (2)
test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck32Test.cs (5)
Fact(15-21)Fact(23-37)Fact(39-61)Fact(63-95)Fact(97-112)test/Cuemon.Core.Tests/Security/CyclicRedundancyCheck64Test.cs (6)
Fact(15-21)Fact(23-37)Fact(39-61)Fact(63-95)Fact(97-108)Fact(110-121)
src/Cuemon.Core/Security/CyclicRedundancyCheck64.cs (1)
src/Cuemon.Core/Validator.cs (1)
Validator(15-1150)
src/Cuemon.Core/Security/CyclicRedundancyCheck.cs (2)
src/Cuemon.Core/Security/CyclicRedundancyCheck32.cs (2)
PolynomialIndexInitializer(46-49)PolynomialSlotCalculator(28-39)src/Cuemon.Core/Security/CyclicRedundancyCheck64.cs (2)
PolynomialIndexInitializer(28-31)PolynomialSlotCalculator(38-49)
test/Cuemon.Core.Tests/Security/FowlerNollVo256Test.cs (3)
test/Cuemon.Core.Tests/Security/FowlerNollVo1024Test.cs (1)
IndependentFNVMultiWord(38-77)test/Cuemon.Core.Tests/Security/FowlerNollVo128Test.cs (1)
IndependentFNVMultiWord(42-81)src/Cuemon.Core/Convertible.cs (2)
Convertible(14-487)ReverseEndianness(456-469)
⏰ 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). (3)
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
test/Cuemon.Core.Tests/Security/FowlerNollVo32Test.cs (1)
1-94: Well-structured tests with comprehensive coverage.This test file provides excellent coverage for the 32-bit FNV implementation, validating both FNV-1 and FNV-1a algorithms, as well as both big-endian and little-endian byte ordering. The independent reference implementation is a good approach for verifying correctness.
test/Cuemon.Core.Tests/Security/FowlerNollVo1024Test.cs (1)
12-36: Test logic is correct; consider adding FNV-1 and endianness coverage.The constructor, empty hash, and non-empty hash tests are well-implemented and validate correctness against the independent reference. For consistency with
FowlerNollVo32Test, consider adding tests for the FNV-1 algorithm variant and little-endian byte order.test/Cuemon.Core.Tests/Security/FowlerNollVo128Test.cs (1)
19-40: Good endianness coverage; FNV-1 algorithm test would complete the suite.The empty hash test validates both big-endian (default) and little-endian configurations, which is better coverage than some other bit-width tests. Adding a test case for
FowlerNollVoAlgorithm.Fnv1would ensure both algorithm variants work correctly for 128-bit hashes.test/Cuemon.Core.Tests/Security/FowlerNollVo512Test.cs (1)
12-36: Tests validate core functionality correctly.The constructor, empty hash length, and non-empty hash tests properly verify the 512-bit FNV implementation against the independent reference. The test structure follows the established pattern across the FNV test suite.
test/Cuemon.Core.Tests/Security/FowlerNollVo256Test.cs (1)
27-36: Good coverage of the 256-bit multi-word implementation vs. independent referenceThe non-empty hash test correctly configures FNV-1a, runs the production
FowlerNollVo256implementation, and verifies the result against an independent BigInteger-based reference. This is a solid oracle-style test that should catch regressions in the 256-bit multi-word path.src/Cuemon.Core/Security/CyclicRedundancyCheck32.cs (1)
59-63: Defensive null-check and Length-based iteration look solidUsing
Validator.ThrowIfNull(input)gives a clearArgumentNullExceptionfor null inputs, and switching toinput.Lengthis idiomatic and equivalent forbyte[]while keeping the loop simple and efficient.src/Cuemon.Core/Security/CyclicRedundancyCheck64.cs (1)
59-63: Consistent null-guard and loop bound with CRC32 implementationMirroring CRC32 by adding
Validator.ThrowIfNull(input)and iterating withinput.Lengthkeeps the 64-bit implementation defensive and idiomatic without altering the hash for valid inputs.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/copilot-instructions.md (1)
59-59: Minor wording improvement."Exact same" is slightly wordy; consider using "the same" or "identical" instead.
- then the corresponding unit test class must use the exact same namespace: + then the corresponding unit test class must use the same namespace:test/Cuemon.Core.Tests/Security/HashFactoryTest.cs (2)
258-266: Default FNV factory test is clear; small assertion-ordering nitThis nicely pins down that
CreateFnv()maps to the 32‑bit implementation. For slightly clearer failure messages, consider usingCreateFnv32as the expected value andCreateFnvas the actual inAssert.Equal(expected first, actual second in xUnit), but behavior is fine as‑is.
268-290: Good coverage of NonCryptoAlgorithm→factory mapping; could be DRY’d with a theoryThis test thoroughly verifies that each
NonCryptoAlgorithm.Fnv*case returns the same result as its dedicated factory. If you want to reduce repetition, consider a[Theory]with(NonCryptoAlgorithm, Func<IHashFactory>)pairs, but that’s optional and the current explicit form is perfectly readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/copilot-instructions.md(1 hunks).github/prompts/benchmark.prompt.md(1 hunks).github/workflows/ci-pipeline.yml(2 hunks)Directory.Packages.props(2 hunks)test/Cuemon.Core.Tests/Security/HashFactoryTest.cs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md
[style] ~59-~59: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...responding unit test class must use the exact same namespace: ```csharp namespace ...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.18.1)
.github/prompts/benchmark.prompt.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.github/copilot-instructions.md
156-156: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
157-157: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
183-183: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
184-184: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
185-185: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
186-186: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
187-187: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
250-250: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
⏰ 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). (3)
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
.github/workflows/ci-pipeline.yml (3)
65-99: YAML structure and matrix inheritance look correct.The matrix strategy is properly defined, job dependencies are correctly declared, and the dynamic
runs-onsubstitution uses correct GitHub Actions syntax. Once runner availability is confirmed, this change will enable broader cross-platform test coverage.
88-88: The Windows runner nameswindows-2025andwindows-11-armare both valid and officially available on GitHub Actions. No issues found.The verification confirms that both runner labels are legitimate and documented in the official GitHub Actions runner reference:
windows-2025is an x64-based runner (Windows Server 2025)windows-11-armis an ARM64-based runnerThe original concerns about these runners being unavailable or non-standard are incorrect. The workflow configuration is valid and will not fail due to runner name issues.
71-71: The specified runner names are all valid and available as GitHub-hosted runners — no issue exists.Based on verification:
ubuntu-24.04-armandwindows-11-armare official GitHub-hosted arm64 runners (generally available since August 7, 2025) for public repositorieswindows-2025is an official GitHub-hosted x64 Windows runner- No custom runner configuration exists in the repository; all referenced runners are standard GitHub-hosted options
The repository (codebeltnet/cuemon) is public, so these runners are available and will not cause pipeline failures.
.github/copilot-instructions.md (1)
543-582: Comprehensive and well-documented FowlerNollVoOptions example.The FowlerNollVoOptions example demonstrates proper XML documentation for the new configuration class introduced in this PR. The documentation clearly outlines the default constructor behavior, property descriptions, and a useful table documenting initial property values. This aligns well with the PR's objective to add public FowlerNollVoOptions and support multiple FNV hash variants.
test/Cuemon.Core.Tests/Security/HashFactoryTest.cs (1)
292-303: CRC64 parameterized/preset parity test looks solidThis is a good safeguard to ensure the explicit polynomial/initial/xor/reflect configuration stays in lockstep with
CreateCrc64GoIso(). The use of the same input and normalized hex casing makes the intent clear; no issues from my side.Directory.Packages.props (1)
7-8: LGTM! Package updates support the new benchmarking infrastructure.The addition of BenchmarkDotNet packages (v0.15.6) aligns with the PR objectives to introduce performance benchmarking capabilities, and the xunit patch version updates (3.2.0 → 3.2.1) are routine maintenance.
Also applies to: 19-20
.github/prompts/benchmark.prompt.md (1)
1-165: Excellent benchmark documentation!This comprehensive guide provides clear conventions for writing performance benchmarks in Cuemon. The structure covers all essential aspects: naming conventions, BenchmarkDotNet configuration, best practices, and concrete examples. This will serve as valuable reference documentation for the new benchmarking infrastructure introduced in this PR.
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 (6)
src/Cuemon.Core/Configuration/IPostConfigurableParameterObject.cs(1 hunks)src/Cuemon.Core/Validator.cs(1 hunks)test/Cuemon.Core.Tests/Assets/FailPostConfigurableOptions.cs(1 hunks)test/Cuemon.Core.Tests/Assets/PostConfigurableOptions.cs(1 hunks)test/Cuemon.Core.Tests/Reflection/AssemblyDecoratorExtensionsTest.cs(1 hunks)test/Cuemon.Core.Tests/ValidatorTest.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/Cuemon.Core.Tests/Reflection/AssemblyDecoratorExtensionsTest.cs
🧰 Additional context used
🧬 Code graph analysis (5)
test/Cuemon.Core.Tests/Assets/PostConfigurableOptions.cs (2)
src/Cuemon.Core/Configuration/IPostConfigurableParameterObject.cs (1)
PostConfigureOptions(11-11)src/Cuemon.Core/Validator.cs (1)
Validator(15-1151)
src/Cuemon.Core/Validator.cs (3)
src/Cuemon.Core/Configuration/IPostConfigurableParameterObject.cs (1)
PostConfigureOptions(11-11)test/Cuemon.Core.Tests/Assets/FailPostConfigurableOptions.cs (1)
PostConfigureOptions(13-15)test/Cuemon.Core.Tests/Assets/PostConfigurableOptions.cs (1)
PostConfigureOptions(14-17)
src/Cuemon.Core/Configuration/IPostConfigurableParameterObject.cs (2)
test/Cuemon.Core.Tests/Assets/FailPostConfigurableOptions.cs (1)
PostConfigureOptions(13-15)test/Cuemon.Core.Tests/Assets/PostConfigurableOptions.cs (1)
PostConfigureOptions(14-17)
test/Cuemon.Core.Tests/Assets/FailPostConfigurableOptions.cs (2)
test/Cuemon.Core.Tests/Assets/PostConfigurableOptions.cs (3)
PostConfigurableOptions(6-23)PostConfigurableOptions(8-10)PostConfigureOptions(14-17)src/Cuemon.Core/Configuration/IPostConfigurableParameterObject.cs (1)
PostConfigureOptions(11-11)
test/Cuemon.Core.Tests/ValidatorTest.cs (3)
test/Cuemon.Core.Tests/Assets/PostConfigurableOptions.cs (2)
PostConfigurableOptions(6-23)PostConfigurableOptions(8-10)src/Cuemon.Core/Validator.cs (2)
Validator(15-1151)ThrowIfInvalidOptions(90-103)test/Cuemon.Core.Tests/Assets/FailPostConfigurableOptions.cs (2)
FailPostConfigurableOptions(5-16)FailPostConfigurableOptions(7-9)
⏰ 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). (3)
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
src/Cuemon.Core/Validator.cs (1)
95-95: LGTM! Post-configuration integration is clean.The post-configuration step is correctly integrated into the validation flow, executing before
ValidateOptions()and within the existing exception handling.test/Cuemon.Core.Tests/ValidatorTest.cs (1)
187-202: LGTM! Tests validate post-configuration lifecycle.Both tests appropriately cover the post-configuration behavior: one verifying successful initialization and validation, the other confirming that failure to initialize results in the expected exception.
src/Cuemon.Core/Configuration/IPostConfigurableParameterObject.cs (1)
1-13: LGTM! Interface design is clean and well-documented.The interface appropriately extends
IParameterObjectand provides a clear lifecycle hook for post-configuration logic. The documentation clearly explains its purpose.test/Cuemon.Core.Tests/Assets/PostConfigurableOptions.cs (1)
6-23: LGTM! Clean test asset demonstrating post-configuration pattern.The class effectively demonstrates the post-configuration lifecycle:
PostConfigureOptions()initializes required state (Id), andValidateOptions()verifies that initialization occurred. Thevirtualmodifier onPostConfigureOptions()appropriately allows derived test classes to override the behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/copilot-instructions.md (2)
156-187: Resolve outstanding markdown formatting violations.These markdown style issues were flagged in the previous review and remain unfixed:
- Line 160: Use atx-style heading (
##) instead of setext underline- Line 169: Add language identifier to fenced code block (should be
```csharp)- Lines 183–187: Change 4-space indentation to 2-space for list items
Apply this diff to fix the heading and code block:
- # Writing Performance Tests in Cuemon - This document provides guidance for writing performance tests (benchmarks) in the Cuemon codebase using BenchmarkDotNet. Follow these guidelines to keep benchmarks consistent, readable, and comparable. + ## Writing Performance Tests in Cuemon + + This document provides guidance for writing performance tests (benchmarks) in the Cuemon codebase using BenchmarkDotNet. Follow these guidelines to keep benchmarks consistent, readable, and comparable.For the code block language identifier:
- ``` - namespace Cuemon.Security.Cryptography + ```csharp + namespace Cuemon.Security.CryptographyFor the list indentation (lines 183–187):
- - Use `BenchmarkDotNet` attributes to express intent and collect relevant metrics: - - `[MemoryDiagnoser]` to capture memory allocations. - - `[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)]` to group related benchmarks. - - `[Params]` for input sizes or variations to exercise multiple scenarios. - - `[GlobalSetup]` for one-time initialization that's not part of measured work. - - `[Benchmark]` on methods representing measured operations; consider `Baseline = true` and `Description` to improve report clarity. + - Use `BenchmarkDotNet` attributes to express intent and collect relevant metrics: + - `[MemoryDiagnoser]` to capture memory allocations. + - `[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)]` to group related benchmarks. + - `[Params]` for input sizes or variations to exercise multiple scenarios. + - `[GlobalSetup]` for one-time initialization that's not part of measured work. + - `[Benchmark]` on methods representing measured operations; consider `Baseline = true` and `Description` to improve report clarity.
254-254: Fix setext-style heading at line 254 to match atx convention.Line 254 uses a setext-style heading (underline), which should be converted to an atx-style heading (
##) for consistency with the document.- # Writing XML documentation in Cuemon - - This document provides instructions for writing XML documentation. + ## Writing XML documentation in Cuemon + + This document provides instructions for writing XML documentation.
🧹 Nitpick comments (4)
.nuget/Cuemon.AspNetCore/PackageReleaseNotes.txt (1)
1-5: Document significant enhancements and refactoring in release notes.The release notes for Version 10.1.0 currently document only dependency upgrades. However, per the PR objectives, this release includes significant changes:
- DateSpan structure refactoring with improved hash/equality logic and extracted helper methods
- Fowler–Noll–Vo hash implementation enhancements supporting multiple bit sizes (32–1024)
- Codebase cleanup (GlobalSuppressions.cs modifications)
While dependency updates are important, the release notes should also highlight these key improvements to inform users about enhancements and any potential impact. Consider adding relevant entries to the # Improvements or # New Features sections if these changes are visible to Cuemon.AspNetCore consumers, or clarify whether these changes are internal-only.
.nuget/Cuemon.Extensions.Data/PackageReleaseNotes.txt (1)
1-6: Release notes entry is minimal; consider adding details about notable changes.The new 10.1.0 entry follows the established format and is consistent with 10.0.0. However, the ALM-only note omits the feature and refactoring improvements mentioned in the PR summary (DateSpan refactoring, FNV hash enhancements, and internal optimizations).
Consider adding a new category (e.g.,
# IMPROVEDor# CHANGED - Internal) to document these changes for clarity and completeness, especially if they may affect users' performance characteristics or upgrade considerations.Optionally, you may add a section like:
Version 10.1.0 Availability: .NET 10, .NET 9 and .NET Standard 2.0 # ALM - CHANGED Dependencies have been upgraded to the latest compatible versions for all supported target frameworks (TFMs) + +# IMPROVED +- REFACTORED DateSpan structure to improve maintainability and readability +- ENHANCED Fowler–Noll–Vo hash implementation with support for multiple bit sizes (32, 64, 128, 256, 512, 1024)This is optional and depends on your release notes documentation policy.
.github/copilot-instructions.md (1)
59-59: Remove redundant wording in line 59.The phrase "exact same namespace" is redundant; "same namespace" is sufficient.
- then the corresponding unit test class must use the exact same namespace: + then the corresponding unit test class must use the same namespace:src/Cuemon.Security.Cryptography/AesCryptor.cs (1)
86-101: AES TransformFinalBlock refactor looks sound; consider behavior verification and a small refactorThe new path that configures
Aes, selects an encryptor/decryptor, and callsTransformFinalBlock(value, 0, value.Length)is correct for a byte[]‑based API and should be faster and simpler than the previous MemoryStream/CryptoStream orchestration.A couple of follow‑ups to consider:
- If earlier implementations trimmed trailing zeros after decryption (as hinted in the PR summary), this refactor now returns the raw decrypted bytes as produced by AES +
Padding. That is generally more correct, but it is an observable behavior change. Please double‑check callers (and tests) that might have relied on implicit trimming, especially when using non‑default paddings (e.g.,PaddingMode.ZerosorPaddingMode.None).- For readability, you can remove one nesting level by switching to
using varand an early return:- using (var transform = mode == AesMode.Encrypt - ? aes.CreateEncryptor() - : aes.CreateDecryptor()) - { - return transform.TransformFinalBlock(value, 0, value.Length); - } + using var transform = mode == AesMode.Encrypt + ? aes.CreateEncryptor() + : aes.CreateDecryptor(); + + return transform.TransformFinalBlock(value, 0, value.Length);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
.github/copilot-instructions.md(1 hunks).nuget/Cuemon.AspNetCore.App/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.AspNetCore.Authentication/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.AspNetCore.Mvc/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.AspNetCore.Razor.TagHelpers/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.AspNetCore/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Core.App/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Core/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Data.Integrity/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Data.SqlClient/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Data/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Diagnostics/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore.Authentication/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore.Mvc.Formatters.Text.Json/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore.Mvc.Formatters.Xml/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore.Mvc.RazorPages/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore.Mvc/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore.Text.Json/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore.Xml/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.AspNetCore/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Collections.Generic/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Collections.Specialized/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Core/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Data.Integrity/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Data/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.DependencyInjection/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Diagnostics/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Hosting/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.IO/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Net/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Reflection/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Runtime.Caching/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Text.Json/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Text/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Threading/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Extensions.Xml/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.IO/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Net/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Resilience/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Runtime.Caching/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Security.Cryptography/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Threading/PackageReleaseNotes.txt(1 hunks).nuget/Cuemon.Xml/PackageReleaseNotes.txt(1 hunks)CHANGELOG.md(1 hunks)Directory.Packages.props(2 hunks)src/Cuemon.Security.Cryptography/AesCryptor.cs(2 hunks)
✅ Files skipped from review due to trivial changes (15)
- .nuget/Cuemon.Resilience/PackageReleaseNotes.txt
- .nuget/Cuemon.Core.App/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.Collections.Generic/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.AspNetCore.Mvc/PackageReleaseNotes.txt
- .nuget/Cuemon.Threading/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.Text.Json/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.DependencyInjection/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.AspNetCore.Xml/PackageReleaseNotes.txt
- .nuget/Cuemon.AspNetCore.Mvc/PackageReleaseNotes.txt
- .nuget/Cuemon.AspNetCore.Authentication/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.Xml/PackageReleaseNotes.txt
- .nuget/Cuemon.Security.Cryptography/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.Core/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.AspNetCore.Text.Json/PackageReleaseNotes.txt
- .nuget/Cuemon.Extensions.Reflection/PackageReleaseNotes.txt
🧰 Additional context used
🪛 LanguageTool
.nuget/Cuemon.Core/PackageReleaseNotes.txt
[grammar] ~15-~15: Ensure spelling is correct
Context: ...fiers, reducing overhead and allocations - OPTMIZED CyclicRedundancyCheck class in the Cuem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
.github/copilot-instructions.md
[style] ~59-~59: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...responding unit test class must use the exact same namespace: ```csharp namespace ...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.18.1)
.github/copilot-instructions.md
156-156: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
157-157: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
183-183: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
184-184: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
185-185: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
186-186: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
187-187: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
250-250: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
⏰ 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). (3)
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (26)
.nuget/Cuemon.Extensions.AspNetCore/PackageReleaseNotes.txt (1)
1-6: Release notes update is consistent and appropriate.The new Version 10.1.0 section is well-formatted, follows the established structure of existing entries, and accurately documents the dependency upgrades that accompany this release. The availability and ALM information align with the PR objectives.
.nuget/Cuemon.Extensions.AspNetCore.Authentication/PackageReleaseNotes.txt (1)
1-6: Version 10.1.0 entry is well-formatted and consistent.The new release notes entry follows the established format and properly documents the dependency upgrades for this package. The availability statement and ALM section align with the PR's broader package update scope.
.nuget/Cuemon.Extensions.Collections.Specialized/PackageReleaseNotes.txt (1)
1-5: Clarify version numbering: PR branch names v10.0.1 but release notes show 10.1.0.The release notes entry uses semantic versioning 10.1.0 (minor version bump), while the PR branch is named
v10.0.1/ai-assisted-performance-boost(patch version). Given the scope of changes (DateSpan refactoring, FNV multi-width hash support, CI enhancements, unit tests), a minor version bump (10.1.0) is more appropriate than a patch (10.0.1). However, verify this is intentional and consistent with your versioning strategy.Format and content are otherwise well-structured and consistent with the rest of the release notes document.
.nuget/Cuemon.Extensions.Data/PackageReleaseNotes.txt (1)
1-2: Verify version numbering alignment with PR branch.The release notes show version 10.1.0, but the PR branch is named "v10.0.1/ai-assisted-performance-boost". Confirm that version 10.1.0 is the intended release version for this PR and that version numbering aligns with your semantic versioning strategy.
.nuget/Cuemon.Extensions.Threading/PackageReleaseNotes.txt (1)
1-5: Release notes entry is well-formatted and accurate.The new Version 10.1.0 entry follows the established format, correctly states the supported TFMs (.NET 10, .NET 9, and .NET Standard 2.0), and appropriately documents the dependency upgrade. The minimal content aligns with the AI summary indicating no functional or public API changes to the Threading package itself.
.nuget/Cuemon.Extensions.AspNetCore.Mvc.Formatters.Text.Json/PackageReleaseNotes.txt (1)
1-5: Release notes entry is well-formatted and consistent.The new Version 10.1.0 entry follows the established format, includes proper availability information, and correctly documents the dependency upgrade. The entry is properly positioned (newest version first) with consistent spacing.
.nuget/Cuemon.Extensions.Runtime.Caching/PackageReleaseNotes.txt (1)
1-5: Release notes format and structure look good.The new 10.1.0 entry follows the established convention and accurately documents the dependency upgrades. Ensure that the DateSpan and FNV hash enhancements mentioned in the PR don't include user-facing API changes for this specific package that should be documented here.
Please confirm that the DateSpan refactoring and FNV hash enhancements from this PR don't affect the public API surface of
Cuemon.Extensions.Runtime.Caching, or if they do, consider adding a brief note to the release notes (e.g., under a separate category like# IMPROVEDor# CHANGED)..nuget/Cuemon.Extensions.AspNetCore.Mvc.Formatters.Xml/PackageReleaseNotes.txt (1)
1-6: Release notes format and placement are correct; verify completeness of documented changes.The new Version 10.1.0 entry follows the established format, is properly placed in reverse-chronological order, and documents dependency upgrades. However, the PR objectives mention significant changes to DateSpan and FNV hashing elsewhere in the codebase. Please verify whether any of these changes directly impact this package's public API or functionality, and if so, ensure they are documented in the release notes beyond the generic dependency upgrade note.
.nuget/Cuemon.IO/PackageReleaseNotes.txt (1)
1-5: No action needed. The release notes adequately document the changes in Cuemon.IO for version 10.1.0—specifically, dependency upgrades to the latest compatible versions. The notes clearly identify the scope (ALM section) and the action taken. Without evidence of code-level modifications to Cuemon.IO itself, the current entry is sufficient..nuget/Cuemon.AspNetCore.Razor.TagHelpers/PackageReleaseNotes.txt (1)
1-5: The release notes for this package are accurate.The mentioned features (FNV hash enhancements, DateSpan refactoring, post-configuration support) are part of the Cuemon.Core 10.1.0 release, not Cuemon.AspNetCore.Razor.TagHelpers. This package is a tag helper utility library with no source code changes in 10.1.0—only dependency upgrades, which are correctly documented. Adding unrelated features to the release notes would mislead users about what actually changed in this package.
Likely an incorrect or invalid review comment.
.nuget/Cuemon.Runtime.Caching/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes block for Version 10.1.0 with correct availability statement and ALM note indicating dependency upgrades.
.nuget/Cuemon.Extensions.IO/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes with correct availability statement (including .NET Standard 2.1 support appropriate for IO extensions) and coordinated dependency upgrade note.
.nuget/Cuemon.Net/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes block with appropriate availability statement and coordinated dependency upgrade note.
.nuget/Cuemon.Extensions.Hosting/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes with correct availability and coordinated dependency upgrade note. Existing 9.0.1 breaking change entry correctly preserved.
.nuget/Cuemon.Extensions.Text/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes with correct availability statement and coordinated dependency upgrade note.
.nuget/Cuemon.AspNetCore.App/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes with appropriate availability statement (limited to .NET versions, suitable for AspNetCore packages) and coordinated dependency upgrade note.
.nuget/Cuemon.Data.Integrity/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes with correct availability statement and coordinated dependency upgrade note.
.nuget/Cuemon.Extensions.Diagnostics/PackageReleaseNotes.txt (1)
1-6: Approved: Consistent 10.1.0 release notes addition.Properly formatted release notes with correct availability and coordinated dependency upgrade note. Existing 9.0.0 breaking change entry correctly preserved.
.nuget/Cuemon.Extensions.Net/PackageReleaseNotes.txt (1)
1-6: 10.1.0 entry matches established release-notes pattern.Availability TFMs and the ALM note about dependency upgrades are consistent with prior entries and the PR’s dependency bumping scope.
.nuget/Cuemon.Xml/PackageReleaseNotes.txt (1)
1-6: Consistent 10.1.0 release header for Cuemon.Xml.New entry cleanly advertises TFMs and dependency refresh in line with other packages; no additional notes needed here.
.nuget/Cuemon.Data.SqlClient/PackageReleaseNotes.txt (1)
1-6: 10.1.0 notes for Cuemon.Data.SqlClient are clear and minimal.The new version header and ALM bullet accurately scope this as a dependency-alignment release without implying behavioral changes.
.nuget/Cuemon.Data/PackageReleaseNotes.txt (1)
1-6: Service update wording for 10.1.0 is aligned with the rest of the suite.Availability and “dependencies upgraded” ALM entry match the cross-package narrative for this minor release.
.nuget/Cuemon.Extensions.AspNetCore.Mvc.RazorPages/PackageReleaseNotes.txt (1)
1-6: RazorPages 10.1.0 entry is TFM-consistent.Limiting Availability to .NET 10/9 and stating dependency upgrades matches prior entries for this package and the overall 10.1.0 story.
.nuget/Cuemon.Diagnostics/PackageReleaseNotes.txt (1)
1-6: Diagnostics 10.1.0 release block is coherent with CHANGELOG messaging.States TFMs and dependency refresh without overspecifying behavior, which fits the per-assembly release-notes convention.
CHANGELOG.md (1)
9-33: 10.1.0 changelog entry is detailed and matches the code-level intent.The new section clearly calls out
IPostConfigurableParameterObject, Validator post-configuration behavior, and the targeted performance changes (FowlerNollVoHash, DateSpan, RandomString, DelimitedString, AesCryptor, CRC, Patterns, SHA-512), giving consumers a precise view of impact for a minor version.Directory.Packages.props (1)
7-8: Centralized test/benchmark dependency bumps look coherent.BenchmarkDotNet (core + Diagnostics.Windows) are kept in sync at 0.15.8, and xUnit core/console runner are aligned at 3.2.1, which should avoid version skew across tests and new benchmarks. Just ensure all benchmark and test projects consume these centrally managed versions without local overrides.
Also applies to: 19-20
gimlichael
changed the title
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 (3)
.github/prompts/benchmark.prompt.md(1 hunks).nuget/Cuemon.Core/PackageReleaseNotes.txt(1 hunks)test/Cuemon.Core.Tests/Security/HashResultTest.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .nuget/Cuemon.Core/PackageReleaseNotes.txt
- test/Cuemon.Core.Tests/Security/HashResultTest.cs
⏰ 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). (3)
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: Analyze (csharp)
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
Directory.Build.props (1)
80-82: Use selective warning suppression instead of blanket-disabling for test projectsSetting
<WarningLevel>0</WarningLevel>globally suppresses all compiler warnings in CI/CLI builds. Instead, suppress specific noisy rules using<NoWarn>or<WarningsNotAsErrors>for test-specific diagnostics. This preserves visibility of real issues—such as deprecated APIs or breaking changes—while avoiding noise from test-specific patterns. Document which warnings are suppressed and why so technical debt remains visible.reports/tuning/Cuemon.DelimitedStringBenchmark-report-github.md (1)
1-16: Tweak fenced block and spacing to satisfy markdownlint (MD040/MD058).To quiet markdownlint without affecting rendering, you can annotate the code fence with a language and insert a blank line before the table:
-``` +```text @@ -PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250ms MaxIterationCount=20 -MinIterationCount=15 WarmupCount=1 - -``` -| Method | Job | Runtime | Count | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Gen1 | Allocated | Alloc Ratio | +PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250ms MaxIterationCount=20 +MinIterationCount=15 WarmupCount=1 + +```text + +| Method | Job | Runtime | Count | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Gen1 | Allocated | Alloc Ratio |If these files are regenerated by tooling, alternatively consider excluding
reports/tuning/**from markdownlint.reports/tuning/Cuemon.Security.FowlerNollVoHashBenchmark-report-github.md (1)
1-16: Align fenced block and table spacing with markdownlint rules.Same as the other reports, you can either fix the markdown or relax linting for generated artifacts:
-``` +```text @@ -MinIterationCount=15 WarmupCount=1 - -``` -| Method | Job | Runtime | Algorithm | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio | +MinIterationCount=15 WarmupCount=1 + +```text + +| Method | Job | Runtime | Algorithm | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |If this file is regenerated by BenchmarkDotNet, excluding
reports/tuning/**from markdownlint may be simpler.reports/tuning/Cuemon.Security.CyclicRedundancyCheckBenchmark-report-github.md (1)
1-16: Consider small markdown fixes or lint exclusions for generated CRC report.Markdownlint’s MD040/MD058 warnings can be addressed with minimal edits:
-``` +```text @@ -MinIterationCount=15 WarmupCount=1 - -``` -| Method | Job | Runtime | Size | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Gen1 | Gen2 | Allocated | Alloc Ratio | +MinIterationCount=15 WarmupCount=1 + +```text + +| Method | Job | Runtime | Size | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Gen1 | Gen2 | Allocated | Alloc Ratio |Otherwise, consider configuring markdownlint to ignore these generated reports.
reports/tuning/Cuemon.GenerateBenchmark-report-github.md (1)
1-16: Keep generated GenerateBenchmark report markdownlint‑clean with minor tweaks.To align with MD040/MD058 while preserving layout:
-``` +```text @@ -MinIterationCount=15 WarmupCount=1 - -``` -| Method | Job | Runtime | Count | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio | +MinIterationCount=15 WarmupCount=1 + +```text + +| Method | Job | Runtime | Count | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |If these reports are frequently regenerated, excluding
reports/tuning/**from markdownlint might be more maintainable.reports/tuning/Cuemon.Security.Cryptography.Sha512256Benchmark-report-github.md (1)
1-36: Align fenced block and table spacing with markdownlint rulesThe content looks fine, but markdownlint’s MD040/MD058 findings are valid here:
- Line 1: Fence has no language (
```). Consider specifying one (e.g.,text) so linters and renderers can treat it correctly.- Lines 13–14: The table header immediately follows the closing fence. Add a blank line between the closing ``` and the
| Method ...line so the table isn’t considered part of the fence and MD058 is satisfied.Example patch:
-``` +```text ... -``` +``` + | Method | Job | Runtime | Variant | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |tuning/Cuemon.Security.Cryptography.Benchmarks/Sha512256Benchmark.cs (1)
14-105: Avoid re-running explicit benchmarks for Variant values they don’t useRight now
AlgorithmVariant Variantis marked with[Params], but onlyParamBased_ComputeHashactually branches onVariant. BenchmarkDotNet will still run all benchmark methods (CustomSHA512256_*andBuiltInSHA512_Truncated_*) for eachVariantvalue, even though those methods hard‑code the algorithm via_factories[...].That means:
- The “Custom”/“Built-in” benchmarks are effectively duplicated for each
Variant, inflating run time.- Result tables can be misleading, since the
Variantcolumn changes while the underlying implementation doesn’t.Consider one of these options:
- Move
Variant+ParamBased_ComputeHashinto a separate benchmark class that’s grouped by params, and keep this class for the explicit small/large methods without[Params]; or- Remove
[Params]fromVarianthere and drive the param-based comparisons through a dedicated param-only benchmark type.This keeps the explicit benchmarks single-sourced per algorithm and avoids confusing
Variantgroupings in the reports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
Cuemon.sln(4 hunks)Directory.Build.props(5 hunks)Directory.Packages.props(1 hunks)reports/tuning/Cuemon.DateSpanBenchmark-report-github.md(1 hunks)reports/tuning/Cuemon.DelimitedStringBenchmark-report-github.md(1 hunks)reports/tuning/Cuemon.GenerateBenchmark-report-github.md(1 hunks)reports/tuning/Cuemon.Security.Cryptography.AesCryptorBenchmarks-report-github.md(1 hunks)reports/tuning/Cuemon.Security.Cryptography.Sha512256Benchmark-report-github.md(1 hunks)reports/tuning/Cuemon.Security.CyclicRedundancyCheckBenchmark-report-github.md(1 hunks)reports/tuning/Cuemon.Security.FowlerNollVoHashBenchmark-report-github.md(1 hunks)reports/tuning/Cuemon.Security.HashResultBenchmark-report-github.md(1 hunks)tuning/Cuemon.Core.Benchmarks/Cuemon.Core.Benchmarks.csproj(1 hunks)tuning/Cuemon.Core.Benchmarks/DateSpanBenchmark.cs(1 hunks)tuning/Cuemon.Core.Benchmarks/DelimitedStringBenchmark.cs(1 hunks)tuning/Cuemon.Core.Benchmarks/GenerateBenchmark.cs(1 hunks)tuning/Cuemon.Core.Benchmarks/Security/CyclicRedundancyCheckBenchmark.cs(1 hunks)tuning/Cuemon.Core.Benchmarks/Security/FowlerNollVoHashBenchmark.cs(1 hunks)tuning/Cuemon.Core.Benchmarks/Security/HashResultBenchmark.cs(1 hunks)tuning/Cuemon.Security.Cryptography.Benchmarks/AesCryptorBenchmarks.cs(1 hunks)tuning/Cuemon.Security.Cryptography.Benchmarks/Cuemon.Security.Cryptography.Benchmarks.csproj(1 hunks)tuning/Cuemon.Security.Cryptography.Benchmarks/Sha512256Benchmark.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tuning/Cuemon.Core.Benchmarks/DateSpanBenchmark.cs (1)
src/Cuemon.Core/DateSpan.cs (1)
GetWeeks(200-214)
tuning/Cuemon.Core.Benchmarks/DelimitedStringBenchmark.cs (1)
src/Cuemon.Core/DelimitedString.cs (2)
DelimitedString(12-118)Split(60-82)
tuning/Cuemon.Security.Cryptography.Benchmarks/AesCryptorBenchmarks.cs (1)
src/Cuemon.Security.Cryptography/AesCryptor.cs (2)
Encrypt(68-71)Decrypt(79-82)
tuning/Cuemon.Core.Benchmarks/GenerateBenchmark.cs (1)
src/Cuemon.Core/Generate.cs (3)
Generate(18-219)FixedString(132-135)ObjectPortrayal(46-70)
tuning/Cuemon.Core.Benchmarks/Security/FowlerNollVoHashBenchmark.cs (1)
tuning/Cuemon.Core.Benchmarks/Security/HashResultBenchmark.cs (2)
MemoryDiagnoser(7-61)GlobalSetup(17-24)
🪛 LanguageTool
reports/tuning/Cuemon.Security.HashResultBenchmark-report-github.md
[grammar] ~15-~15:
Context: ... | |------------------------------------ |---------- |---------- |----- |--------...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...---------------------------- |---------- |---------- |----- |---------------:|---...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...---------------- |---------- |---------- |----- |---------------:|--------------:...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...--------- |---------- |---------- |----- |---------------:|--------------:|------...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...------ |---------- |----- |---------------:|--------------:|--------------:|------...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...-- |----- |---------------:|--------------:|--------------:|---------------:|-----...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...----------:|--------------:|--------------:|---------------:|---------------:|----...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...---------:|--------------:|---------------:|---------------:|---------------:|----...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...--------:|---------------:|---------------:|---------------:|------:|--------:|---...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~15-~15:
Context: ...--------:|---------------:|---------------:|------:|--------:|-------:|-------:|--...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_CONTRACTION)
[grammar] ~16-~16: Zin met fouten
Context: ...-:| | 'HashResult.GetBytes - copy bytes' | .NET 10.0 | .NET 10.0 | **...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~17-~17: Interpunctie toevoegen
Context: ...| - | NA | | 'HashResult.GetBytes - copy bytes' | .NET 9.0 ...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~17-~17: Zin met fouten
Context: ...A** | | 'HashResult.GetBytes - copy bytes' | .NET 9.0 | .NET 9.0 | 0 | ...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~31-~31: Interpunctie toevoegen
Context: ... | | | | 'HashResult.To<string> (converter)' | .NET...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~34-~34: Interpunctie toevoegen
Context: ... | | | | 'HashResult.GetBytes - copy bytes' | **.NET 1...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~34-~34: Zin met fouten
Context: ... | | 'HashResult.GetBytes - copy bytes' | .NET 10.0 | .NET 10.0 | **...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~35-~35: Zin met fouten
Context: ...0** | | 'HashResult.GetBytes - copy bytes' | .NET 9.0 | .NET 9.0 | 8 | ...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~49-~49: Interpunctie toevoegen
Context: ... | | | | 'HashResult.To<string> (converter)' | .NET...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~52-~52: Interpunctie toevoegen
Context: ... | | | | 'HashResult.GetBytes - copy bytes' | **.NET 1...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~52-~52: Zin met fouten
Context: ... | | 'HashResult.GetBytes - copy bytes' | .NET 10.0 | .NET 10.0 | **...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~53-~53: Zin met fouten
Context: ...0** | | 'HashResult.GetBytes - copy bytes' | .NET 9.0 | .NET 9.0 | 32 | ...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~67-~67: Interpunctie toevoegen
Context: ... | | | | 'HashResult.To<string> (converter)' | .NET...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~70-~70: Interpunctie toevoegen
Context: ... | | | | 'HashResult.GetBytes - copy bytes' | **.NET 1...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~70-~70: Zin met fouten
Context: ... | | 'HashResult.GetBytes - copy bytes' | .NET 10.0 | .NET 10.0 | **...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~71-~71: Zin met fouten
Context: ...0** | | 'HashResult.GetBytes - copy bytes' | .NET 9.0 | .NET 9.0 | 256 | 3...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~82-~82: Interpunctie toevoegen
Context: ... | | | | | HashResult.ToBinaryString | .NET 10.0 | ....
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~85-~85: Interpunctie toevoegen
Context: ... | | | | 'HashResult.To<string> (converter)' | .NET...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~88-~88: Interpunctie toevoegen
Context: ... | | | | 'HashResult.GetBytes - copy bytes' | **.NET 1...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~88-~88: Zin met fouten
Context: ... | | 'HashResult.GetBytes - copy bytes' | .NET 10.0 | .NET 10.0 | **...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~89-~89: Zin met fouten
Context: ...0** | | 'HashResult.GetBytes - copy bytes' | .NET 9.0 | .NET 9.0 | 1024 | 15...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~103-~103: Interpunctie toevoegen
Context: ... | | | | 'HashResult.To<string> (converter)' | .NET...
(QB_NEW_NL_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
🪛 markdownlint-cli2 (0.18.1)
reports/tuning/Cuemon.Security.HashResultBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
reports/tuning/Cuemon.Security.CyclicRedundancyCheckBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
reports/tuning/Cuemon.Security.Cryptography.Sha512256Benchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
reports/tuning/Cuemon.DateSpanBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
reports/tuning/Cuemon.DelimitedStringBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
reports/tuning/Cuemon.GenerateBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
reports/tuning/Cuemon.Security.Cryptography.AesCryptorBenchmarks-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
reports/tuning/Cuemon.Security.FowlerNollVoHashBenchmark-report-github.md
1-1: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ 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). (3)
- GitHub Check: call-build (Release) / 🛠️ Build
- GitHub Check: call-build (Debug) / 🛠️ Build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (13)
tuning/Cuemon.Core.Benchmarks/Cuemon.Core.Benchmarks.csproj (1)
1-9: Benchmark project wiring looks correctMinimal SDK project with only a RootNamespace and a reference to
Cuemon.Coreis appropriate here; TFMs and BenchmarkDotNet dependencies are correctly delegated to shared build props viaIsBenchmarkProject.Cuemon.sln (2)
3-4: Visual Studio version bump is benign but verify team toolingUpdating the solution header to
# Visual Studio Version 18/VisualStudioVersion = 18.0...is fine, but it may cause older VS installations to prompt for an upgrade path. Confirm this is intentional for your team’s supported tooling.
173-178: New tuning benchmark projects are consistently wired into the solutionThe
tuningsolution folder, the two benchmark projects, their configuration mappings, and NestedProjects entries are all consistent (paths, GUIDs, and parent folder). This should integrate the new benchmark projects cleanly into the solution.Also applies to: 501-508, 593-594
tuning/Cuemon.Core.Benchmarks/Security/HashResultBenchmark.cs (1)
1-61: HashResult benchmark covers the right surface and setup is correctGlobalSetup, parameterization, and the set of benchmarked APIs (
GetBytes,To*variants, converter-basedTo) are coherent and side‑effect free; this is a solid benchmark fixture.Directory.Build.props (2)
112-119: Benchmark project configuration viaIsBenchmarkProjectis cleanly separatedConditioning benchmark TFMs (
net10.0;net9.0) and BenchmarkDotNet PackageReferences onIsBenchmarkProjectkeeps benchmark concerns isolated from main source/test projects and works well with the new*.Benchmarksprojects.
4-6: FixIsSourceProject/IsToolingProjectproperty definitions and correct directory referenceThe
IsSourceProjectandIsToolingProjectconditions use unnecessary nested.ToLower()calls and can be simplified. More critically,IsToolingProjectreferences a non-existent"tooling"directory; the actual directory is named"tuning"and is reserved for benchmarks (already handled byIsBenchmarkProject). Either rename the directory to"tooling"if intended for tool projects, or remove theIsToolingProjectdefinition if no tooling projects are planned.For simplification without behavioral change:
- <IsSourceProject>$(MSBuildProjectDirectory.ToLower().StartsWith('$(MSBuildThisFileDirectory.ToLower())src'))</IsSourceProject> - <IsToolingProject>$(MSBuildProjectDirectory.ToLower().StartsWith('$(MSBuildThisFileDirectory.ToLower())tooling'))</IsToolingProject> + <IsSourceProject>$(MSBuildProjectDirectory.StartsWith('$(MSBuildThisFileDirectory)src'))</IsSourceProject> + <IsToolingProject>$(MSBuildProjectDirectory.StartsWith('$(MSBuildThisFileDirectory)tooling'))</IsToolingProject>If a
tooling/directory is not planned, remove theIsToolingProjectdefinition and its associatedPropertyGroupat line 84–88 to avoid dead code.tuning/Cuemon.Security.Cryptography.Benchmarks/Cuemon.Security.Cryptography.Benchmarks.csproj (1)
1-9: Crypto benchmarks project is correctly minimalRoot namespace and a single reference to
Cuemon.Security.Cryptographyare all that’s needed; TFMs and BenchmarkDotNet dependencies are rightly centralized via shared props for*.Benchmarksprojects.tuning/Cuemon.Core.Benchmarks/DelimitedStringBenchmark.cs (1)
12-53: DelimitedString benchmarks look correct and representative.Setup correctly exercises both quoted and unquoted CSV‑like fields, and the
Create/Splitbenchmarks useDelimitedString.Create/Splitwith appropriateDelimiter/Qualifierconfiguration. The null‑forgiving initialization plus[GlobalSetup]pattern is standard for BenchmarkDotNet; no functional issues stand out here.tuning/Cuemon.Security.Cryptography.Benchmarks/AesCryptorBenchmarks.cs (1)
11-38: AesCryptor benchmarks are well‑structured and isolate crypto costs.The GlobalSetup correctly derives a key/IV, seeds deterministic plaintext, and precomputes ciphertext so
EncryptandDecrypteach benchmark a single operation. Resource handling (using var aes) is safe, and the BenchmarkDotNet pattern is idiomatic. No changes needed.tuning/Cuemon.Core.Benchmarks/DateSpanBenchmark.cs (1)
26-92: DateSpanBenchmark provides solid coverage of the updated DateSpan API.The setup fixes a single
_nowpoint, derives varied span lengths, and uses ISO‑sortable strings withCultureInfo.InvariantCulture, which aligns with theDateSpan.Parseoverloads. The benchmarks cleanly cover construction, parsing, formatting,GetWeeks, hashing, and equality/operator behavior without hidden side effects.tuning/Cuemon.Core.Benchmarks/Security/CyclicRedundancyCheckBenchmark.cs (1)
8-52: CRC benchmarks look clean and deterministicThe setup and benchmark methods are well-structured:
- Fixed RNG seed in
Setupensures deterministic payloads across runs.- Warm-up calls on
_crc32/_crc64keep lazy initialization out of the measured path.- Stream benchmarks correctly wrap the shared payload in a non-writable
MemoryStreamper invocation, avoiding accidental mutation.No changes requested here.
tuning/Cuemon.Core.Benchmarks/Security/FowlerNollVoHashBenchmark.cs (1)
7-90: FNV benchmarks are wired correctly to the Algorithm parameterThe benchmark wiring looks solid:
Algorithmis exposed via[Params]and consumed inGlobalSetupwhen constructing eachFowlerNollVoXXinstance, so both Fnv1 and Fnv1a are covered.- Payload construction is done once in
GlobalSetup, and all benchmark methods are thin wrappers overComputeHashon the appropriate hasher, which is ideal for BenchmarkDotNet.No issues from my side.
tuning/Cuemon.Core.Benchmarks/GenerateBenchmark.cs (1)
8-103: Generate benchmarks are representative and side‑effect safeThis benchmark class nicely exercises the
GenerateAPIs:
Countcovers small/mid/large cases.RangeOf_Enumerateuses a private_sinkto keep enumeration work from being optimized away.- Other benchmarks call straight into
Generatehelpers with fixed inputs, making results easy to compare across runs.I don’t see any functional or structural issues here.
reports/tuning/Cuemon.Security.Cryptography.AesCryptorBenchmarks-report-github.md
Show resolved
Hide resolved
|
gimlichael
had a problem deploying
to
Production
GitHub Actions
Failure
This pull request introduces significant refactoring and improvements to the
DateSpanstructure and the Fowler–Noll–Vo hash implementation. The main changes include extracting complex date difference calculations into helper methods for better readability and maintainability, improving hash code generation forDateSpanby using a more robust algorithm, and enhancing the Fowler–Noll–Vo hash to support multiple bit sizes and optimize performance for larger hashes.DateSpan structure improvements
DateSpanby moving calculations for average days per year, month/day/hour/millisecond differences, and partial month/time differences into dedicated helper methods (CalculateAverageDaysPerYear,CalculateDifference,CalculatePartialMonthDifference,CalculateTimeDifference). This makes the code much easier to follow and maintain. [1] [2]DateSpanby switching to a Fowler–Noll–Vo 64-bit hash, incorporating calendar type identification, and using calendar IDs for equality checks. This ensures more robust and consistent hash codes and equality logic. [1] [2] [3]_calendartoCalendarin XML comments. [1] [2]Fowler–Noll–Vo hash enhancements
ToUInt32LittleEndian,MultiplyMod32) to support large hash sizes efficiently.Codebase cleanup
GlobalSuppressions.cs, reflecting improvements in code readability and maintainability.Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.