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

Shout.c : Add a missing c++14 fall through comment #4860

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

daschuer
Copy link
Member

The source is compiled wit the default c++14 and uses /* fall through */ to silence the -Wimplicit-fallthrough=3 enabled by -Wextra.

@@ -1308,6 +1308,7 @@ static int try_connect(shout_t *self)
}
#endif
self->state = SHOUT_STATE_REQ_CREATION;
/* fall through */
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we use the c++17 attribute? https://en.cppreference.com/w/cpp/language/attributes/fallthrough

Suggested change
/* fall through */
[[falltrough]];

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this for the lowest impact.
I am afraid switching to c++17 may break something else.
All other places in the source use this comment and it is not our code.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@Swiftb0y
Copy link
Member

There are still a bunch of other warnings related to libshout. I assume you don't intend to fix them?

@daschuer
Copy link
Member Author

I can have a look if there are low hanging fruits.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@Swiftb0y Swiftb0y merged commit 9ba11db into mixxxdj:main Jul 19, 2022
@daschuer daschuer deleted the shout_fallthough branch November 16, 2022 08:34
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.

2 participants