Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream_info: Make some struct members private #34058

Merged
merged 5 commits into from May 15, 2024

Conversation

abeyad
Copy link
Contributor

@abeyad abeyad commented May 9, 2024

No description provided.

abeyad added 2 commits May 9, 2024 17:34
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
@abeyad abeyad closed this May 9, 2024
@kyessenov
Copy link
Contributor

Why close? I think this is a good fix.

@abeyad abeyad reopened this May 10, 2024
@abeyad
Copy link
Contributor Author

abeyad commented May 10, 2024

Why close? I think this is a good fix.

Hey @kyessenov thanks! I ran into some issues w/ the PR and wasn't sure when I would be able to get back to it, so closed it, but I just tried a fix so re-opening the PR now.

@abeyad abeyad requested a review from kyessenov May 10, 2024 19:52
@abeyad abeyad assigned abeyad and kyessenov and unassigned abeyad May 10, 2024
@ravenblackx
Copy link
Contributor

@kyessenov ping

absl::InlinedVector<ResponseFlag, 4> response_flags_{};
bool health_check_request_{};
Router::RouteConstSharedPtr route_;
envoy::config::core::v3::Metadata metadata_{};
FilterStateSharedPtr filter_state_;

private:
Copy link
Contributor

@kyessenov kyessenov May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the order of the fields the same? This is a fairly common data structure and the layout would change if we re-order - possibly for worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests have a size check which passed:

sizeof(stream_info) == 840 || sizeof(stream_info) == 856 || sizeof(stream_info) == 888 ||
sizeof(stream_info) == 776 || sizeof(stream_info) == 728 || sizeof(stream_info) == 744 ||
sizeof(stream_info) == 680 || sizeof(stream_info) == 696 || sizeof(stream_info) == 688 ||
sizeof(stream_info) == 736 || sizeof(stream_info) == 728 || sizeof(stream_info) == 712 ||
sizeof(stream_info) == 704)
, so I assumed the ordering was fine, but given they check a bunch of different sizes, I reverted back to the same ordering as before. PTAL.

Signed-off-by: Ali Beyad <abeyad@google.com>
@abeyad
Copy link
Contributor Author

abeyad commented May 15, 2024

/retest

@alyssawilk alyssawilk merged commit 517413a into envoyproxy:main May 15, 2024
53 checks passed
@abeyad abeyad deleted the stream_info_private branch May 15, 2024 17:38
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.

None yet

4 participants