brbzull0 commented on code in PR #13635:
URL: https://github.com/apache/trafficserver/pull/13635#discussion_r3956794193
##########
src/proxy/http3/Http3Transaction.cc:
##########
@@ -225,6 +225,13 @@ HQTransaction::transaction_done()
int
HQTransaction::get_transaction_id() const
+{
+ // Narrowing is intentional here; see get_quic_stream_id() for the
full-width value.
+ return static_cast<int>(this->_stream_id);
+}
Review Comment:
Copilot asked for this cast in its Sep 4 comment on this line — "make the
truncation explicit to avoid accidental sign/width issues" — so I'd rather not
churn it back on the same grounds.
More to the point, ATS already relies on this conversion. `Http2StreamId` is
`uint32_t`, `Http2Stream::_id` is initialized to `-1` (`Http2Stream.h:217`),
and `Http2ConnectionState::set_stream_id()` uses `stream->get_transaction_id()
< 0` as its unassigned sentinel (`Http2ConnectionState.cc:1704`). That works
only because `uint32_t(0xFFFFFFFF)` narrows to `int(-1)`. HTTP/2 also does the
same narrowing with no cast at all: `Http2Stream::get_transaction_id()` is
`return _id;`.
On the standard: `QUICStreamId` is `uint64_t`, and this file builds as core,
where the standard is C++20. P1236R1 rewrote `[conv.integral]/3` on top of
mandatory two's complement and deleted the signed/unsigned split, so the result
is the unique destination-type value congruent to the source modulo 2^N —
well-defined, not implementation-defined. It was implementation-defined in
C++17, so the concern was fair for that standard. The one platform dependence
left is the width of `int` itself, which the standard leaves
implementation-defined with a 16-bit minimum; it's 32 everywhere ATS runs. Note
that `static_cast<int>(static_cast<uint32_t>(x))` is bit-identical for any
`int` narrower than 33 bits, and diverges from the direct cast only on a
64-bit-`int` target — where the intermediate step is the one that loses the
modular guarantee.
The `int` return isn't ours to widen:
`ProxyTransaction::get_transaction_id()` is pure virtual with that signature,
and covariant return doesn't apply to scalars.
A clamp or assert would change the behavior of a log field, and no
`numeric_cast` helper exists in ATS or swoc to reach for. The first negative
value appears at stream id 2^31; client bidi ids advance by 4, so that's
roughly 2^29 — about 537 million streams on a single connection. After this PR
nothing routes, indexes, or compares on this value; it feeds debug output
(formatted `PRIx32`, so unsigned regardless) and the `ctid` log field. Leaving
as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
