Skip to content

Change MBED_STATIC_ASSERTs version for built-in #13085

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
Merged
merged 1 commit into from
Nov 24, 2020
Merged

Change MBED_STATIC_ASSERTs version for built-in #13085

merged 1 commit into from
Nov 24, 2020

Conversation

pea-pod
Copy link
Contributor

@pea-pod pea-pod commented Jun 9, 2020

Summary of changes

Changes all of the MBED_STATIC_ASSERT and MBED_STRUCT_STATIC_ASSERT instances to static_assert and includes assert.h for the the C files.

In PR #13002 both MBED_STATIC_ASSERT and MBED_STRUCT_STATIC_ASSERT were deprecated. The macros now just call in place static_assert (or the C equivalent _Static_assert) without modification. This removes reliance on the MBED[_STRUCT]_STATIC_ASSERT throughout the mbed-os code base.

Impact of changes

None.

Migration actions required

None.

Documentation

PR #13002 deprecates the use of MBED_STATIC_ASSERT and MBED_STRUCT_STATIC_ASSERT as both of these uses are now fully supported by the minimum compiler levels. This PR may go through without any other PR preceding it, but the reasoning behind this removal comes from #13002.

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] 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)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@mergify mergify bot added the needs: work label Jun 9, 2020
@pea-pod
Copy link
Contributor Author

pea-pod commented Jun 9, 2020

While looking around in the code base for MBED_STATIC_ASSERT and MBED_STRUCT_STATIC_ASSERT instances, I found some in UNITTESTS\target_h\platform\mbed_assert.h.

Inside this, I found redefinitions of both MBED_STATIC_ASSERT and MBED_STRUCT_STATIC_ASSERT on lines 79 through 132 (including doxygen comments).

I was not sure what to do with this, since I am far outside of my comfort zone on the tests themselves. @kjbracey-arm @0xc0170 any thoughts? I could pull these in to this or create a new one. I could even modify #13002 even though I don't want to touch that. Still, that could possibly be the Right Thing To Do™. It does seem odd regarding the UNITTEST files duplicating the asserts completely.

@ciarmcom ciarmcom requested review from a team June 9, 2020 05:00
@ciarmcom
Copy link
Member

ciarmcom commented Jun 9, 2020

@pea-pod, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-pan please review.

@kjbracey
Copy link
Contributor

kjbracey commented Jun 9, 2020 •
edited
Loading

If you're stripping them out of the code base, then you won't need the deprecated definitions for building unit tests.

You could either bring the two copies of mbed_assert.h into closer alignment - copy the relevant changes over, minimising the diff - or you could just remove them. No strong opinion.

No point modifying 13002.

@adbridge
Copy link
Contributor

@bulislaw are you happy with this change?

bulislaw
bulislaw previously approved these changes Jun 16, 2020
View reviewed changes
Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

LGTM

@pea-pod pea-pod force-pushed the remove-deprecated-mbed-assert branch from db5b4e7 to dfb010e Compare June 16, 2020 18:30
@mergify mergify bot dismissed bulislaw’s stale review June 16, 2020 18:30

Pull request has been modified.

@pea-pod pea-pod force-pushed the remove-deprecated-mbed-assert branch from dfb010e to 9f93dcc Compare June 16, 2020 18:45
@mergify
Copy link

mergify bot commented Jun 18, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 18, 2020

@pea-pod Can you resolve conflicts please?

@pea-pod pea-pod force-pushed the remove-deprecated-mbed-assert branch from 9f93dcc to e4b4a84 Compare June 18, 2020 23:10
@pea-pod
Copy link
Contributor Author

pea-pod commented Jun 18, 2020

@0xc0170 Done.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 19, 2020

CI started

@mergify mergify bot added the needs: work label Jun 19, 2020
0xc0170
0xc0170 previously approved these changes Jun 19, 2020
View reviewed changes
@mbed-ci
Copy link

mbed-ci commented Jun 19, 2020

Test run: FAILED

Summary: 1 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 10, 2020

@pea-pod Thanks, let's wait with a rebase.

@kjbracey-arm you happy as the things are in this PR now?

kjbracey
kjbracey previously approved these changes Oct 1, 2020
View reviewed changes
@pea-pod pea-pod force-pushed the remove-deprecated-mbed-assert branch from 9c67bf6 to 507181d Compare October 27, 2020 13:31
@mergify mergify bot dismissed kjbracey’s stale review October 27, 2020 13:32

Pull request has been modified.

@pea-pod
Copy link
Contributor Author

pea-pod commented Oct 30, 2020

@0xc0170 Is this ready?

@kjbracey kjbracey mentioned this pull request Nov 6, 2020
Closed
evedon
evedon reviewed Nov 19, 2020
View reviewed changes
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.

All previous comments were addressed.
Would be good to restart CI.

platform/cxxsupport/mstd_atomic
@@ -23,7 +23,6 @@
#include <mstd_memory>
#include <mstd_type_traits>
#include <mstd_utility>
#include "platform/mbed_assert.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the include should be kept.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? Just to avoid compilation glitches from people who've been accidentally picking up MBED_ASSERT via an <mstd_atomic> include?

I agree this is a fairly common minor compatibility break - header implementations stopping including undocumented things - but don't recall it being a general rule not to do it.

In practice this is one of the less likely places to cause issues - there aren't many <mstd_atomic> users around, cos it's relatively new. You'd be far more likely to get headaches from losing something in DigitalOut or SerialBase, and I've not noticed reluctance to tidy their includes.

Although having said that, I don't see any other tidies in this PR. Not sure if that's because they couldn't be done or they just haven't been.

@0xc0170 0xc0170 closed this Nov 20, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2020

I'll reopen soon, to start travis (the old job could not be restarted as it was prior migration).

@mergify mergify bot removed needs: CI release-type: patch Indentifies a PR as containing just a patch labels Nov 20, 2020
@0xc0170 0xc0170 reopened this Nov 20, 2020
@0xc0170 0xc0170 added needs: CI release-type: patch Indentifies a PR as containing just a patch labels Nov 20, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2020

continuous-integration/travis-ci/pr — The Travis CI build could not complete due to an error

Needs to be ignored, the new status Travis CI - Pull Request .

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 20, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 24, 2020

I marked this as ready. I'll merge later today

@0xc0170 0xc0170 merged commit 57bbb47 into ARMmbed:master Nov 24, 2020
@mergify mergify bot removed the ready for merge label Nov 24, 2020
@mbedmain mbedmain added release-version: 6.6.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjbracey kjbracey kjbracey left review comments

@evedon evedon evedon left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@bulislaw bulislaw bulislaw left review comments

Assignees
No one assigned
Labels
release-version: 6.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@pea-pod @ciarmcom @kjbracey @adbridge @0xc0170 @mbed-ci @bulislaw @evedon @mbedmain