Skip to content

Address review feedback: TrackOutputBytes sentinel error design is intentional#25

Closed
Copilot wants to merge 2 commits into
reduce-allocationsfrom
copilot/sub-pr-22-another-one
Closed

Address review feedback: TrackOutputBytes sentinel error design is intentional#25
Copilot wants to merge 2 commits into
reduce-allocationsfrom
copilot/sub-pr-22-another-one

Conversation

Copilot AI commented Mar 11, 2026

Copy link
Copy Markdown

Addresses review feedback questioning why ErrOutputBytesExceeded / ErrNegativeTrackingSize are returned directly from reflection-based decode paths rather than wrapped in *UnmarshalError.

Resolution

No code changes required. The design is intentional:

  • Sentinel errors use errors.New (not fmt.Errorf) to keep TrackOutputBytes small enough for the Go compiler to inline
  • Generated code wraps these sentinels via fmt.Errorf before they reach callers
  • The existing comment at the declaration site already documents this contract
// Sentinel errors for TrackOutputBytes — using errors.New instead of
// fmt.Errorf keeps the function small enough for the Go compiler to inline.
// Callers wrap these with fmt.Errorf to add context.
var (
    ErrOutputBytesExceeded  = errors.New("output bytes limit exceeded")
    ErrNegativeTrackingSize = errors.New("negative tracking size")
)

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

tamirms and others added 2 commits March 11, 2026 12:53
… limits

Add a MaxOutputBytes field to DecodeOptions that tracks cumulative
decoded output size across a single decode operation. Before each
allocation (array element, union arm, optional field, opaque data),
the size is added to a running total; if it exceeds MaxOutputBytes
the decode is aborted with ErrOutputBytesExceeded.

Key changes:
- Add TrackOutputBytes method and TrackOutputBytesOf[T] generic helper
- Cap array pre-allocation at 256 elements, growing via append beyond that
- Track allocations in DecodeFixedOpaque, decodeArray, decodeUnion,
  decodeMap, and allocPtrIfNil
- Bump minimum Go version to 1.25 for generics support

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI changed the title [WIP] Update MaxOutputBytes decode option implementation Address review feedback: TrackOutputBytes sentinel error design is intentional Mar 11, 2026
@tamirms tamirms force-pushed the reduce-allocations branch from 112f917 to c677e1b Compare March 11, 2026 18:52
@tamirms

tamirms commented Mar 11, 2026

Copy link
Copy Markdown

Closing — improvements from this PR have been incorporated into #22 where applicable. Thank you!

@tamirms tamirms closed this Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants