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

-Wimplicit-fallthrough generating warnings #121040

Closed
nohlson opened this issue Jun 26, 2024 · 9 comments
Closed

-Wimplicit-fallthrough generating warnings #121040

nohlson opened this issue Jun 26, 2024 · 9 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@nohlson
Copy link
Contributor

nohlson commented Jun 26, 2024 •
edited by bedevere-app bot
Loading

Bug report

Bug description:

Due to the addition of -Wimplicit-fallthrough as a BASEFLAG new warnings are generated.

This should be reverted until tooling is created to track these new warnings per #112301

Warnings can be found in builds https://buildbot.python.org/all/#/builders/721/builds/1465/steps/3/logs/warnings__143_ from #121030

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs
@nohlson nohlson added the type-bug An unexpected behavior, bug, or error label Jun 26, 2024
@bedevere-app bedevere-app bot mentioned this issue Jun 26, 2024
Merged
corona10 pushed a commit that referenced this issue Jun 26, 2024
@nohlson
gh-121040: Remove fallthrough warnings compiler option (gh-121041)
ef28f6d 
Remove fallthrough warnings
@sobolevn
Copy link
Member

A potential fix could look something like:

#if __has_attribute(__fallthrough__)
#  define _Py_FALLTHROUGH __attribute__((__fallthrough__))
#else
#  define _Py_FALLTHROUGH do { } while (0)
#endif

vstinner added a commit to vstinner/cpython that referenced this issue Jun 26, 2024
@vstinner
pythongh-121040: Use __attribute__((fallthrough))
89ca8fd 
Annotate explicitly "fall through" switch cases with a new
_Py_FALLTHROUGH macro which uses __attribute__((fallthrough)) if
available.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 26, 2024
@vstinner
pythongh-121040: Use __attribute__((fallthrough))
bb2eb56 
Annotate explicitly "fall through" switch cases with a new
_Py_FALLTHROUGH macro which uses __attribute__((fallthrough)) if
available.

Fix warnings when using -Wimplicit-fallthrough compiler flag.
@bedevere-app bedevere-app bot mentioned this issue Jun 26, 2024
Merged
@vstinner
Copy link
Member

Previous issues:

@vstinner
Copy link
Member

A potential fix could look something like: (...)

I wrote PR gh-121044 to fix these compiler warnings; the PR adds a new _Py_FALLTHROUGH macro.

@corona10
Copy link
Member

A potential fix could look something like:

It can not be the ultimate solution.

  • Should check MSVC is accept this macro. (or do nothing if the compiler is MSVC)
  • Even if we added the macro for our codebase, vendored libraries like libexpat should follow the new compiler rule.

@vstinner
Copy link
Member

Should check MSVC is accept this macro. (or do nothing if the compiler is MSVC)

Does MSC implement a flag like -Wimplicit-fallthrough? If not, I don't think that we should care about MSC in _Py_FALLTHROUGH macro.

Even if we added the macro for our codebase, vendored libraries like libexpat should follow the new compiler rule.

If you want to use this compiler flag, you can use system libraries, instead of building embedded copies of libexpat and libmpdecimal.

@corona10
Copy link
Member

corona10 commented Jun 26, 2024 •
edited
Loading

If you want to use this compiler flag, you can use system libraries, instead of building embedded copies of libexpat and libmpdecimal.

IIUC, the original intention was to adopt that compiler flag as default, not optional.

@corona10
Copy link
Member

Does MSC implement a flag like -Wimplicit-fallthrough? If not, I don't think that we should care about MSC in _Py_FALLTHROUGH macro.

But it emits the compiler warning so we should provide dummy macro for the MSVC
see: https://github.com/python/cpython/pull/121044/files#r1654967484

@vstinner
Copy link
Member

But it emits the compiler warning so we should provide dummy macro for the MSVC
see: https://github.com/python/cpython/pull/121044/files#r1654967484

You misunderstood the warning. It was only about __has_attribute(). I fixed this issue.

vstinner added a commit that referenced this issue Jun 27, 2024
@vstinner @sobolevn
gh-121040: Use __attribute__((fallthrough)) (#121044)
12af8ec 
Fix warnings when using -Wimplicit-fallthrough compiler flag.

Annotate explicitly "fall through" switch cases with a new
_Py_FALLTHROUGH macro which uses __attribute__((fallthrough)) if
available. Replace "fall through" comments with _Py_FALLTHROUGH.

Add _Py__has_attribute() macro. No longer define __has_attribute()
macro if it's not defined. Move also _Py__has_builtin() at the top
of pyport.h.

Co-Authored-By: Nikita Sobolev <mail@sobolevn.me>
@vstinner
Copy link
Member

Warnings were fixed by 12af8ec. I close the issue.

mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
@nohlson @mrahtz
pythongh-121040: Remove fallthrough warnings compiler option (pythong…
c8152ac 
…h-121041)

Remove fallthrough warnings
mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
@vstinner @sobolevn @mrahtz
pythongh-121040: Use __attribute__((fallthrough)) (python#121044)
638d523 
Fix warnings when using -Wimplicit-fallthrough compiler flag.

Annotate explicitly "fall through" switch cases with a new
_Py_FALLTHROUGH macro which uses __attribute__((fallthrough)) if
available. Replace "fall through" comments with _Py_FALLTHROUGH.

Add _Py__has_attribute() macro. No longer define __has_attribute()
macro if it's not defined. Move also _Py__has_builtin() at the top
of pyport.h.

Co-Authored-By: Nikita Sobolev <mail@sobolevn.me>
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
@nohlson @noahbkim
pythongh-121040: Remove fallthrough warnings compiler option (pythong…
2f01f4d 
…h-121041)

Remove fallthrough warnings
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
@vstinner @sobolevn @noahbkim
pythongh-121040: Use __attribute__((fallthrough)) (python#121044)
0deef4f 
Fix warnings when using -Wimplicit-fallthrough compiler flag.

Annotate explicitly "fall through" switch cases with a new
_Py_FALLTHROUGH macro which uses __attribute__((fallthrough)) if
available. Replace "fall through" comments with _Py_FALLTHROUGH.

Add _Py__has_attribute() macro. No longer define __has_attribute()
macro if it's not defined. Move also _Py__has_builtin() at the top
of pyport.h.

Co-Authored-By: Nikita Sobolev <mail@sobolevn.me>
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
@nohlson @estyxx
pythongh-121040: Remove fallthrough warnings compiler option (pythong…
73a8f4b 
…h-121041)

Remove fallthrough warnings
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
@vstinner @sobolevn @estyxx
pythongh-121040: Use __attribute__((fallthrough)) (python#121044)
f1286b8 
Fix warnings when using -Wimplicit-fallthrough compiler flag.

Annotate explicitly "fall through" switch cases with a new
_Py_FALLTHROUGH macro which uses __attribute__((fallthrough)) if
available. Replace "fall through" comments with _Py_FALLTHROUGH.

Add _Py__has_attribute() macro. No longer define __has_attribute()
macro if it's not defined. Move also _Py__has_builtin() at the top
of pyport.h.

Co-Authored-By: Nikita Sobolev <mail@sobolevn.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@vstinner @sobolevn @corona10 @nohlson