De-reflect + DRY the array/enumerable converters (P4) - #25
Merged
Conversation
Introduce a shared CollectionTypeConverter base that builds collection results with Array.CreateInstance + indexed fill and selects the element converter by walking the concrete LinkedList (struct enumerator) rather than LINQ First. ArrayTypeConverter/EnumerableTypeConverter collapse to thin subclasses that differ only in CanConvert + element-type extraction; both now return T[] (safe: IsEnumerable() matches only IEnumerable<T>, which a T[] satisfies). This drops the per-convert List<T> + its backing array, the reflected Enumerable.ToArray (MakeGenericMethod + Invoke + args array), and the First predicate closure. TypeConverter.CreateNullResult swaps the Enumerable.Empty<T>() reflection for Array.CreateInstance(t, 0), and GetConverter is now a true manual walk (matching its own comment). Also strips a stray UTF-8 BOM from the file. Proof via the new gated ConvertArrayBenchmark (isolates the hot path like Q1/Q3/Q4): 1.33 KB -> 688 B (-49%), 1,247 -> 219 ns (5.7x). The residual 688 B is the split-substrings + per-element boxing + result array shared by both the old and new code. Adds 7 collection-converter parity tests (delimited string -> int[] / string[] / IEnumerable<int>, empty-entry removal, custom delimiter, default-array passthrough, and that the enumerable path materializes a T[]). Suite: 63 tests per TFM (was 56). Also refreshes SESSION-HANDOFF.md + FIX-PLAN.md and carries the pre-P4 post-P3 style tweaks to TypeConverter.cs / TypeExtensions.cs.
Adds 5 collection-converter tests the dotnet code-reviewer suggested: - CreateNullResult null path: an unbound IEnumerable<T> with no default now yields an empty T[] -- the one line P4 changed in TypeConverter.cs that no existing test exercised (all others bind a value or supply a default). - Element-converter parity: DayOfWeek[], DateTime[], Uri[] (the shipped tests only covered int/string, both routed to DefaultTypeConverter). - Negative: a non-numeric element for an int[] surfaces as the expected SettingsPropertyValueException, pinning the exception-wrapping contract. Suite: 68 per TFM (was 63). No production changes. Refreshes SESSION-HANDOFF.md + FIX-PLAN.md (counts, PR #25, code-review outcome).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P4 — de-reflect + DRY the array/enumerable converters
De-reflects the collection conversion hot path and removes the near-duplication between the two collection converters.
What changed
Conversion/CollectionTypeConverter— a shared base implementingConvertonce: normalize the incoming value to an array (split a delimited string / pass an existing array through / wrap a scalar), select the element converter by walking the concreteLinkedList(struct enumerator — no boxed enumerator, no predicate closure), then fill anArray.CreateInstance(elementType, n)by index.ArrayTypeConverter/EnumerableTypeConvertercollapse to thin subclasses that differ only inCanConvert+ element-type extraction. Both now returnT[]— safe becauseIsEnumerable()matches onlyIEnumerable<T>, which aT[]satisfies.List<T>+ its backing array, the reflectedEnumerable.ToArray(MakeGenericMethod+Invoke+ args array), and theFirstpredicate closure.TypeConverter.CreateNullResultswaps theEnumerable.Empty<T>()reflection forArray.CreateInstance(t, 0);GetConverteris now a true manual walk (matching its own comment). Also strips a stray UTF-8 BOM from the file.Proof
New gated
ConvertArrayBenchmark(isolates the hot path like the Q1/Q3/Q4 micro-benchmarks); ShortRun on net10.0:The residual 688 B is the split-substrings + per-element boxing + result array, shared by both the old and new code (irreducible without changing the
object Convert(object, Type)element contract).Tests
+7 collection-converter parity tests (
Conversion/CollectionConversionTests.cs): delimited string →int[]/string[]/IEnumerable<int>, empty-entry removal, custom delimiter, default-array passthrough, and that the enumerable path now materializes aT[]. Suite: 63 per TFM (was 56).Also in this PR
Refreshes
SESSION-HANDOFF.md+FIX-PLAN.md, and carries the pre-P4 post-P3 style tweaks toTypeConverter.cs/TypeExtensions.csthat were already in the working tree meant to ride this branch.