Skip to content

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