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 MBED_FALLTHROUGH attribute #12032

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Dec 5, 2019

Summary of changes

C++17 standardised [[fallthrough]] for switch statements to suppress compiler warnings. Provide access to it, or compiler-specific alternatives.

Impact of changes

MBED_FALLTHROUGH attribute added to mbed_toolchain.h - a portable equivalent for C++17's [[fallthrough]].

Migration actions required

None

Documentation

Toolchain attributes not covered by docs


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@JanneKiiskila


C++17 standardised `[[fallthrough]]` for `switch` statements to suppress
compiler warnings. Provide access to it, or compiler-specific
alternatives.
Copy link
Contributor

@JanneKiiskila JanneKiiskila left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@kjbracey
Copy link
Contributor Author

kjbracey commented Dec 5, 2019

Just to note that the GCC form here requires GCC 7. I believe it will generate an "empty declaration" warning on GCC 6.

I could adjust if pre-GCC 7 compatibility is an issue.

@JanneKiiskila
Copy link
Contributor

Well, Mbed OS uses GCC9 now in master and 5.15. onwards, so I doubt that is an issue.
We just switch the warning (that exists) into a) most cases no warning and b) using an old compiler into a slightly different warning. In both cases - the outcome is just fine, IMHO.

@kjbracey
Copy link
Contributor Author

kjbracey commented Dec 5, 2019

I agree here, but thought it was worth flagging for anyone considering stealing borrowing adapting the code for another environment, like ns_types.h.

@JanneKiiskila
Copy link
Contributor

Hey, it's Apache 2.0, it's fair game!-)

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 5, 2019

CI started

@0xc0170 0xc0170 requested a review from a team December 5, 2019 12:47
@mbed-ci
Copy link

mbed-ci commented Dec 5, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 5, 2019

@ARMmbed/mbed-os-core Once you approve, this is ready to go in

@bulislaw
Copy link
Member

bulislaw commented Dec 6, 2019

That doesn't look like it fixes anything and may be problematic for some users so lets push it for 6.

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed release-version: 5.15.0-rc2 labels Dec 6, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

Moved to 6.0.0 (@JanneKiiskila)

@JanneKiiskila
Copy link
Contributor

@bulislaw - what is problematic about this, can you please explain?

@bulislaw
Copy link
Member

bulislaw commented Dec 9, 2019

We are past code freeze going through RCs, as a rule only bug fixes land in the RCs.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 11, 2019

@ARMmbed/mbed-os-core Once you approve, this is ready to go in

Please review

@adbridge
Copy link
Contributor

@bulislaw could you review this for Core ?

Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

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

It would be good to turn on -Wimplicit-fallthrough at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants