-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix compilation in http_integration.cc #8896
Fix compilation in http_integration.cc #8896
Conversation
Fixes -Werror=maybe-uninitialized Risk Level: Low Testing: yes Docs Changes: n/a Release Notes: n/a Fixes envoyproxy#8894 Signed-off-by: Anatoly Scheglov <[email protected]>
@@ -422,6 +422,7 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryHostPredicateFilter) { | |||
|
|||
// Note how we're expecting each upstream request to hit the same upstream. | |||
auto upstream_idx = waitForNextUpstreamRequest({0, 1}); | |||
ASSERT_LT(upstream_idx, fake_upstreams_.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the only place where the return value was used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead can you have it return an absl::optional? That is more future proof IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing. Small comment.
/wait
@@ -422,6 +422,7 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryHostPredicateFilter) { | |||
|
|||
// Note how we're expecting each upstream request to hit the same upstream. | |||
auto upstream_idx = waitForNextUpstreamRequest({0, 1}); | |||
ASSERT_LT(upstream_idx, fake_upstreams_.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead can you have it return an absl::optional? That is more future proof IMO.
Signed-off-by: Anatoly Scheglov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Fixes -Werror=maybe-uninitialized
Risk Level: Low
Testing: yes
Docs Changes: n/a
Release Notes: n/a
Fixes #8894