Clarify ArrayRecord handling of Multiple Null Records - #132479
Clarify ArrayRecord handling of Multiple Null Records#132479adamsitnik with Copilot wants to merge 1 commit into
Conversation
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
@adamsitnik I think the pertinent question is "why does this check exist?" It's purely a performance optimization, but it comes across as a correctness check. That's also why, in the conversation I had with you, I pushed back a bit against the unit test. Unit tests typically check correctness, and I think most readers and maintainers of the code base would not expect the presence of a performance-check-only unit test. (That deserves a comment in the unit test!) |
There was a problem hiding this comment.
Pull request overview
This PR updates the inline documentation above ArrayRecord.CheckExpectedRecordCount to clarify how NRBF array record counting behaves when “multiple null” records are present, especially when allowNulls is enabled.
Changes:
- Replaced the existing XML doc/remarks block with a shorter two-line comment describing the record-count vs element-count invariant.
- Focused the comment specifically on the special-case behavior of multiple-null records when nulls are allowed.
| // 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. |
There was a problem hiding this comment.
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.
Problem
CheckExpectedRecordCountdescribed record counting with extra detail and mixed concerns; this update narrows it to the exact invariant around Multiple Null Records.Change
Updated snippet