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.

Sign up for GitHub

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

Jump to bottom

quic: address new coverity warning #48384

Closed
wants to merge 1 commit into from
Closed

quic: address new coverity warning #48384

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/quic/session.cc
View file
Open in desktop
Expand Up @@ -363,7 +363,7 @@ Maybe<Session::Options> Session::Options::From(Environment* env,

auto& state = BindingData::Get(env);
auto params = value.As<Object>();
Options options;
Options options = Options();
Copy link
Member

Choose a reason for hiding this comment

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

This seems odd. I might be missing something, but the explicit constructor call Options() should perform the same initialization as the implicit call in Options options, and assignment should not change anything either.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tniessen I'm not 100% sure this will fix the warning as I don't know that we can run coverity locally. I think other similar reports have been resolved by a similar fix through. Do you have an alternate suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tniessen I'll commit to looking to make sure it resolved the report after this lands. If it does not I'll figure out some other change that should. I'm thinking that is the easiest way to figure out if it does/does not unless you have another suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

@mhdawson Sorry, I missed the first ping. This still seems odd to me, especially given that coverity complains about options.transport_params.preferred_address_ipv4._M_payload._M_payload (that is, a field in a class that is not Options), but this PR has the necessary approvals to land and I won't block it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tniessen you were correct. My change did not resolve the Coverity report. Will investigate further to better understand what is being reported.


#define SET(name) \
SetOption<Session::Options, &Session::Options::name>( \
Expand Down