Skip to content

Clarify ArrayRecord handling of Multiple Null Records #132479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −8
Draft

Clarify ArrayRecord handling of Multiple Null Records #132479

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,8 @@ private void HandleNext(object value, NextInfo info, int size)

internal abstract (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetAllowedRecordType();

/// <summary>
/// Checks that the number of records matches the number of array elements when an exact count is expected.
/// </summary>
/// <remarks>
/// Every record represents a single value, except for the Multiple Null Records which represent more than one null.
/// When nulls are allowed, Multiple Null Records are permitted and the record count is not expected to match the total element count.
/// It's important to perform this check before allocating the array, as the array can be very large.
/// </remarks>
// Every record represents a single value, except for the Multiple Null Records which represent more than one null.
// When nulls are allowed, Multiple Null Records are permitted and the record count is not expected to match the total element count.
Comment on lines +117 to +118

@bartonjs bartonjs Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a comment required at all?

If anything, just keep it simple.

// Other than multiple-null, every record represents one object.
// So for null-disallowed, we can do a quick check to avoid unnecessary work.

But my recommendation is just to delete the comment altogether.

private protected void CheckExpectedRecordCount(List<SerializationRecord> records, bool allowNulls)
{
if (!allowNulls && ArrayInfo.FlattenedLength != records.Count)
Expand Down
Loading