Replacing network ConnectionEvent bitfield with enum.#1358
Replacing network ConnectionEvent bitfield with enum.#1358dnoe merged 4 commits intoenvoyproxy:masterfrom
Conversation
dnoe
left a comment
dnoe
left a comment
There was a problem hiding this comment.
Lots of lines but little change. Two quick questions.
| virtual void closeSocket(ConnectionEvent close_type); | ||
| void doConnect(); | ||
| void raiseEvents(uint32_t events); | ||
| void raiseEvents(ConnectionEvent event); |
There was a problem hiding this comment.
Could be renamed raiseEvent to match onEvent (which happened to be wrong before and is correct now :)
There was a problem hiding this comment.
Good call! too bad your reward is another 100 lines of string replaces...
Mostly test code though :-)
| void onEvent(Network::ConnectionEvent event) override { | ||
| closed_ |= (event == Network::ConnectionEvent::RemoteClose || | ||
| event == Network::ConnectionEvent::LocalClose); | ||
| connected_ |= (event == Network::ConnectionEvent::Connected); |
There was a problem hiding this comment.
Hmm, isn't |= a bitwise operator? This doesn't look right to me. I'm not sure it was right before, either?
There was a problem hiding this comment.
I think it's fine if you're using the least significant bit as the boolean value (and are happy with some implicit boolean/integer casting going on under the hood).
There was a problem hiding this comment.
It looks like the standard concretely guarantees that false is 0 and true is 1, so I think it's safe from UB. I think ye olde time C (before C99) only guaranteed that true expressions were non-zero. I guess it still works in that case but looks odd.
I'm OK with leaving it as is.
include/envoy/network/connection.h
Outdated
| static const uint32_t RemoteClose = 0x1; | ||
| static const uint32_t LocalClose = 0x2; | ||
| static const uint32_t Connected = 0x4; | ||
| enum ConnectionEvent { |
There was a problem hiding this comment.
Prefer enum class, more type safe.
There was a problem hiding this comment.
Ooh, good call. Updated this, and a bunch of tests that subsequently failed to compile.
| void onEvent(Network::ConnectionEvent event) override { | ||
| closed_ |= (event == Network::ConnectionEvent::RemoteClose || | ||
| event == Network::ConnectionEvent::LocalClose); | ||
| connected_ |= (event == Network::ConnectionEvent::Connected); |
There was a problem hiding this comment.
I think it's fine if you're using the least significant bit as the boolean value (and are happy with some implicit boolean/integer casting going on under the hood).
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for doing this! LGTM beyond other comments.
|
@alyssawilk random GH tip: If you put "Fixes #912" in the description, that PR will be auto closed when this merges. Anyway, this LGTM other than the enum class comment. |
| } | ||
|
|
||
| ENVOY_CONN_LOG(debug, "closing socket: {}", *this, close_type); | ||
| ENVOY_CONN_LOG(debug, "closing socket: {}", *this, static_cast<uint32_t>(close_type)); |
There was a problem hiding this comment.
Might be worth a << override, up to you. I wish C++ had a better stringification for eum class by default.
Automatic merge from submit-queue. Add the support of forward_payload_header to jwt_auth **What this PR does / why we need it**: Add the support of forward_payload_header to jwt_auth, which specifies the output header of the JWT payload. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#1213 **Special notes for your reviewer**: **Release note**: ```release-note ```
It is required for LTO builds, specifically it fixes stacktrace_unittest. This is because API of stacktrace capturing functions implicitly assumes that those are not inlined. Refers to github issue envoyproxy#1358.
Many our unit tests require specific stack traces or that allocation/deallocation is not elimininated. As compilation gets more aggressive (such as when cross-object optimization is enabled during LTO), we break more of those cases. This commit extends noopt usage that disables inlining optimization in specific cases. This fixes github issue envoyproxy#1358.
…nthropic and VertexAI (#1358) **Description** Ensure the model name from response body is passed on to openAI response body --------- Signed-off-by: Sukumar Gaonkar <[email protected]>
4 participants
Fixes #912