Copilot commented on code in PR #13533:
URL: https://github.com/apache/trafficserver/pull/13533#discussion_r3758810171
##########
src/iocore/net/UnixNet.cc:
##########
@@ -39,9 +39,18 @@ std::atomic<bool> net_memory_throttle = false;
int fds_throttle;
ink_hrtime last_transient_accept_error;
+namespace
+{
+/// Config members that @c NetHandler::configure_per_thread_values reads.
+constexpr unsigned long long PER_THREAD_DEPENDENT_CONFIG{0x3};
+// std::bitset silently discards bits at or above its width, which would drop a
+// member from the set without any diagnostic if Config ever shrinks.
+static_assert(PER_THREAD_DEPENDENT_CONFIG < (1ULL <<
NetHandler::CONFIG_ITEM_COUNT));
Review Comment:
This `static_assert` invokes `1ULL << NetHandler::CONFIG_ITEM_COUNT`, which
becomes undefined/ill-formed if `CONFIG_ITEM_COUNT >= 64` (shift count >= bit
width). To make this robust, either (a) add a separate
`static_assert(NetHandler::CONFIG_ITEM_COUNT < std::numeric_limits<unsigned
long long>::digits)` before shifting, or (b) avoid shifting altogether by
asserting the mask fits via something like `(PER_THREAD_DEPENDENT_CONFIG >>
NetHandler::CONFIG_ITEM_COUNT) == 0`.
##########
include/iocore/net/NetHandler.h:
##########
@@ -130,6 +128,13 @@ class NetHandler : public Continuation, public
EThread::LoopTailHandler
return *(&max_connections_in + n);
}
};
+ // Config is addressed as an array of uint32_t through operator[], and
+ // config_value_affects_per_thread_value is a bitset indexed by field
+ // position, so the offset of each member is part of the interface.
+ static_assert(offsetof(Config, max_connections_in) == 0 * sizeof(uint32_t));
+ static_assert(offsetof(Config, max_requests_in) == 1 * sizeof(uint32_t));
+ static_assert(offsetof(Config, default_inactivity_timeout) == 2 *
sizeof(uint32_t));
Review Comment:
`offsetof` is only well-defined for standard-layout types, and these asserts
are enforcing a very specific ABI/layout contract already. Consider also
asserting `std::is_standard_layout_v<Config>` (so future edits don’t silently
make `offsetof` non-portable) and asserting `sizeof(Config) ==
CONFIG_ITEM_COUNT * sizeof(uint32_t)` to pin both the expected field count and
absence of internal padding relative to the array-like indexing contract.
--
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]
