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

Add and use SDL_FALLTHROUGH for fallthroughs #4791

Merged
merged 1 commit into from Nov 11, 2021

Conversation

InfoTeddy
Copy link
Contributor

@InfoTeddy InfoTeddy commented Sep 27, 2021

Case fallthrough warnings can be suppressed using the __fallthrough__ compiler attribute. Unfortunately, not all compilers have this attribute, or even have __has_attribute to check if they have the __fallthrough__ attribute. [[fallthrough]] is also available in C++17 and the next C2x, but not everyone uses C++17 or C2x.

So define the SDL_FALLTHROUGH macro to deal with those problems - if we are using C++17 or C2x, it expands to [[fallthrough]]; else if the compiler has __has_attribute and has the __fallthrough__ attribute, then it expands to __attribute__((__fallthrough__)); else it expands to an empty statement, with a /* fallthrough */ comment (it's a do {} while (0) statement, because users of this macro need to use a semicolon, because [[fallthrough]] and __attribute__((__fallthrough__)) require a semicolon).

Applications using SDL are also free to use this macro (because it is defined in begin_code.h).

All existing /* fallthrough */ comments have been replaced with this macro. Some of them were unnecessary because they were the last case in a switch; using SDL_FALLTHROUGH in those cases would result in a compile error on compilers that support __fallthrough__, for having a __attribute__((__fallthrough__)) statement that didn't immediately precede a case label.

@InfoTeddy InfoTeddy force-pushed the fallthrough branch 7 times, most recently from ea45029 to 374d551 Compare October 2, 2021 22:22
Case fallthrough warnings can be suppressed using the __fallthrough__
compiler attribute. Unfortunately, not all compilers have this
attribute, or even have __has_attribute to check if they have the
__fallthrough__ attribute. [[fallthrough]] is also available in C++17
and the next C2x, but not everyone uses C++17 or C2x.

So define the SDL_FALLTHROUGH macro to deal with those problems - if we
are using C++17 or C2x, it expands to [[fallthrough]]; else if the
compiler has __has_attribute and has the __fallthrough__ attribute, then
it expands to __attribute__((__fallthrough__)); else it expands to an
empty statement, with a /* fallthrough */ comment (it's a do {} while
(0) statement, because users of this macro need to use a semicolon,
because [[fallthrough]] and __attribute__((__fallthrough__)) require a
semicolon).

Applications using SDL are also free to use this macro (because it is
defined in begin_code.h).

All existing /* fallthrough */ comments have been replaced with this
macro. Some of them were unnecessary because they were the last case in
a switch; using SDL_FALLTHROUGH in those cases would result in a compile
error on compilers that support __fallthrough__, for having a
__attribute__((__fallthrough__)) statement that didn't immediately
precede a case label.
@slouken slouken merged commit 66a08aa into libsdl-org:main Nov 11, 2021
@InfoTeddy InfoTeddy deleted the fallthrough branch November 11, 2021 17:46
@sezero
Copy link
Contributor

sezero commented Nov 11, 2021

This causes multitudes of warning from old clang, e.g. clang-3.4.2:

src/audio/SDL_wave.c:1718:9: warning: declaration does not declare anything [-Wmissing-declarations]
        SDL_FALLTHROUGH;
        ^~~~~~~~~~~~~~~
include/begin_code.h:179:25: note: expanded from macro 'SDL_FALLTHROUGH'
#define SDL_FALLTHROUGH __attribute__((__fallthrough__))
                        ^~~~~~~~~~~~~
src/audio/SDL_wave.c:1857:9: warning: declaration does not declare anything [-Wmissing-declarations]
        SDL_FALLTHROUGH;
        ^~~~~~~~~~~~~~~
include/begin_code.h:179:25: note: expanded from macro 'SDL_FALLTHROUGH'
#define SDL_FALLTHROUGH __attribute__((__fallthrough__))
                        ^~~~~~~~~~~~~
2 warnings generated.

[many many others...]

@slouken
Copy link
Collaborator

slouken commented Nov 11, 2021

Do you have a fix?

@sezero
Copy link
Contributor

sezero commented Nov 11, 2021

Do you have a fix?

unfortunately I don't

slouken added a commit that referenced this pull request Nov 11, 2021
This reverts commit 66a08aa.

This causes problems with older compilers:
#4791 (comment)
@slouken
Copy link
Collaborator

slouken commented Nov 12, 2021

Okay, I reverted it for now. @InfoTeddy, can you look and see the best way to fix this?

@sezero
Copy link
Contributor

sezero commented Nov 12, 2021

FWIW: clang documentation shows that the GNU syntax, i.e. __attribute__((__fallthrough__)) is supported as of clang-10.0. The warnings will thus show with clang-9.x and older.
https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html#fallthrough
https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html#fallthrough

@InfoTeddy
Copy link
Contributor Author

Should be as simple as expanding to __attribute__((__fallthrough__)) only if Clang is 10 or later and if GCC is 7 or later, otherwise do {} while (0) /* fallthrough */.

@slouken
Copy link
Collaborator

slouken commented Nov 12, 2021

Do you want to create a new pull request for that?

@InfoTeddy
Copy link
Contributor Author

Sure, I can do that.

@InfoTeddy
Copy link
Contributor Author

Done: #4944

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

3 participants