Add XML doc comments for the IEEE 754 decimal types and value properties#131084
Conversation
|
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. |
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
There was a problem hiding this comment.
Pull request overview
This PR fills missing XML documentation on the public surface area for the IEEE 754 decimal floating-point types Decimal32, Decimal64, and Decimal128 in System.Numerics, including type-level summaries and the standard “value” properties (for example, PositiveInfinity, NaN, MinValue, Epsilon).
Changes:
- Add XML
<summary>docs toDecimal32,Decimal64, andDecimal128describing the IEEE 754decimalNinterchange format and precision. - Add XML
<summary>docs for the static value properties (PositiveInfinity,NegativeInfinity,NaN,NegativeZero,Zero,MinValue,MaxValue,Epsilon) matching the canonical interface wording.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal32.cs | Adds type and value-property XML docs for Decimal32 consistent with existing One/Pi/etc. summaries. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal64.cs | Adds type and value-property XML docs for Decimal64 using canonical interface wording. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs | Adds type and value-property XML docs for Decimal128 using canonical interface wording. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "0d8bc909d52bd77b16d772312526b6f269af6858",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "ea3f7f141e0596cab37785d305910e64d031ab29",
"last_reviewed_commit": "0d8bc909d52bd77b16d772312526b6f269af6858",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "ea3f7f141e0596cab37785d305910e64d031ab29",
"last_recorded_worker_run_id": "29758465340",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "c65102a96b81512ae4afa1c20442d63c1bfdd7c4",
"review_id": 4736501663
},
{
"commit": "0d8bc909d52bd77b16d772312526b6f269af6858",
"review_id": 4736820822
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The IEEE 754 decimal interchange types Decimal32, Decimal64, and Decimal128 had gaps in their XML documentation on the public surface. The type declarations themselves and the static value properties (PositiveInfinity, NegativeInfinity, NaN, NegativeZero, Zero, MinValue, MaxValue, Epsilon) were undocumented, producing incomplete API docs and CS1591 gaps for these public members.
Approach: The PR adds <summary> XML doc comments only. Each type gets a one-line summary describing that it uses the IEEE 754 decimalN interchange format with binary integer decimal (BID) encoding and its precision (7, 16, 34 digits). Each static value property reuses the canonical summary wording from the interface it implements (IFloatingPointIeee754<TSelf>, IMinMaxValue<TSelf>, INumberBase<TSelf>), matching the file's existing convention of literal summaries for members like One/Pi/Tau. There are no functional changes.
Summary: This is a low-risk, documentation-only change. I verified each added property summary against the corresponding interface definitions (IFloatingPointIeee754.cs, IMinMaxValue.cs, INumberBase.cs) and confirmed the text matches verbatim, including the slightly awkward but canonical Epsilon phrasing ("Gets the smallest value such that can be added to 0 that does not result in 0."). The wording follows the repository's docs guidance (present-tense third-person verbs, <c> for inline code). The three files are changed consistently. The remaining undocumented members are explicit implementations of the internal IDecimalIeee754ParseAndFormatInfo<T, TSignificand> interface, which are not public surface and correctly left undocumented, consistent with Half/BFloat16. No functional risk, no build/test impact beyond doc generation. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 45.4 AIC · ⌖ 15.9 AIC · ⊞ 10K
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
b5c2805 to
0d8bc90
Compare
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal64.cs:15
- The claims the interchange encoding (BID vs DPD) is determined by the platform ABI. This type’s implementation appears to assume a fixed BID-style representation (no DPD encode/decode paths), so mentioning ABI-dependent encoding is likely misleading for a managed type. Consider stating the actual encoding used (BID) or dropping the ABI sentence.
/// <summary>
/// Represents a decimal floating-point number that uses the IEEE 754 <c>decimal64</c> interchange format, providing 16 decimal digits of precision.
/// </summary>
/// <remarks>The IEEE 754 standard defines two interchange encodings for decimal floating-point: binary integer decimal (BID) and densely packed decimal (DPD). Which encoding is used is determined by the underlying ABI for the platform and defaults to BID where the ABI does not otherwise specify.</remarks>
src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal32.cs:16
- The claims the interchange encoding (BID vs DPD) is determined by the platform ABI. This type’s implementation appears to assume a fixed BID-style representation (no DPD encode/decode paths), so mentioning ABI-dependent encoding is likely misleading for a managed type. Consider stating the actual encoding used (BID) or dropping the ABI sentence.
/// <summary>
/// Represents a decimal floating-point number that uses the IEEE 754 <c>decimal32</c> interchange format, providing 7 decimal digits of precision.
/// </summary>
/// <remarks>The IEEE 754 standard defines two interchange encodings for decimal floating-point: binary integer decimal (BID) and densely packed decimal (DPD). Which encoding is used is determined by the underlying ABI for the platform and defaults to BID where the ABI does not otherwise specify.</remarks>
src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs:15
- The claims the interchange encoding (BID vs DPD) is determined by the platform ABI. This type’s implementation appears to assume a fixed BID-style representation (no DPD encode/decode paths), so mentioning ABI-dependent encoding is likely misleading for a managed type. Consider stating the actual encoding used (BID) or dropping the ABI sentence.
/// <summary>
/// Represents a decimal floating-point number that uses the IEEE 754 <c>decimal128</c> interchange format, providing 34 decimal digits of precision.
/// </summary>
/// <remarks>The IEEE 754 standard defines two interchange encodings for decimal floating-point: binary integer decimal (BID) and densely packed decimal (DPD). Which encoding is used is determined by the underlying ABI for the platform and defaults to BID where the ABI does not otherwise specify.</remarks>
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Holistic Review
Motivation: The IEEE 754 decimal interchange types Decimal32, Decimal64, and Decimal128 had documentation gaps on their public surface: the type declarations and the static value properties (PositiveInfinity, NegativeInfinity, NaN, NegativeZero, Zero, MinValue, MaxValue, Epsilon) were undocumented, producing incomplete API docs and CS1591 gaps.
Approach: The PR adds XML doc comments only, with no functional changes. Each static value property reuses the canonical summary wording from the interface it implements (IFloatingPointIeee754<TSelf>, IMinMaxValue<TSelf>, INumberBase<TSelf>), matching the file's existing literal-summary convention. In the current head the type-level documentation was refined: the previous single-line <summary> that asserted the type uses "the binary integer decimal (BID) encoding" was changed to a shorter summary that only names the decimalN interchange format and precision, and a new <remarks> was added explaining that IEEE 754 defines two interchange encodings (BID and DPD) and that the encoding is determined by the platform ABI, defaulting to BID where unspecified.
Summary: This remains a low-risk, documentation-only change. The incremental edit improves accuracy: hard-coding "BID encoding" into the summary overstated a guarantee, and moving the encoding discussion into <remarks> with the ABI caveat is a more correct characterization of the interchange format concept for the public docs. The property summaries are unchanged and still match their interface definitions verbatim, including the canonical Epsilon phrasing. The three files remain consistent with each other, and the wording follows the repository's docs guidance (present-tense third-person verbs, <c> for inline code). No functional risk and no build/test impact beyond doc generation. LGTM.
Detailed Findings
No actionable findings. The only change since the prior review is the type-level summary/remarks refinement described above, which is an improvement.
Assessment History
- review 4736501663 reviewed commit
c65102a96b81with verdict LGTM. Current verdict: LGTM. Assessment unchanged in verdict, motivation, approach, and risk; the incremental patch only rewords the type-level documentation (moving the BID/DPD encoding detail from the summary into a new<remarks>), which does not alter the doc-only, low-risk nature of the PR.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 44.4 AIC · ⌖ 9.65 AIC · ⊞ 10K
vcsjones
left a comment
There was a problem hiding this comment.
Looks good to me. This is locally consistent with other things in the runtime repository.
Fills in the remaining XML doc gaps on the public surface of
Decimal32,Decimal64, andDecimal128.decimalNinterchange format with the binary integer decimal (BID) encoding and their respective precision (7, 16, and 34 decimal digits).PositiveInfinity,NegativeInfinity,NaN,NegativeZero,Zero,MinValue,MaxValue, andEpsilon. These now correspond toIFloatingPointIeee754<TSelf>/IMinMaxValue<TSelf>/INumberBase<TSelf>members, so the summaries use the interfaces'' canonical wording verbatim, matching the file''s existing convention of literal summaries forOne/E/Pi/Tau/NegativeOne.The remaining undocumented members are explicit implementations of the
internalIDecimalIeee754ParseAndFormatInfo<T, TSignificand>interface, which aren''t part of the public surface and don''t require docs (consistent withHalf/BFloat16).Doc-comment-only change.
Note
This PR description was drafted by Copilot.