Skip to content

Fully enforce NonCopyable #12581

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
Apr 9, 2020
Merged

Fully enforce NonCopyable #12581

merged 1 commit into from
Apr 9, 2020
+1 −45

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Mar 5, 2020

Summary of changes

Make NonCopyable fully operational so it gives compile errors in all build profiles.

This removes the deprecated copy operators which let code compile with a warning.

Impact of changes

Code that copies non-copyable classes will now not compile in any profile - previously a link error would have been generated in debug profile, but other profiles would have only generated a "deprecated" warning at compile time and a debug message at runtime.

Migration actions required

Modify code to not copy non-copyable objects. Any code doing so would have likely led to problems such as memory leaks.

Documentation

No changes - documentation already suggests this is how it operates.

Pull request type 

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[X] 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

@ciarmcom ciarmcom requested review from a team March 5, 2020 16:00
@ciarmcom
Copy link
Member

ciarmcom commented Mar 5, 2020

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@adbridge
Copy link
Contributor

@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-core could we please have a review on this ?

evedon
evedon reviewed Mar 10, 2020
View reviewed changes
@mergify
Copy link

mergify bot commented Mar 12, 2020

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

bulislaw
bulislaw previously approved these changes Mar 13, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: work labels Mar 13, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 16, 2020

CI started

@mergify mergify bot added needs: work and removed needs: CI labels Mar 16, 2020
@mbed-ci
Copy link

mbed-ci commented Mar 16, 2020

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

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

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 17, 2020

Looks like an example has a problem, will try to fix it.

0xc0170 added a commit to ARMmbed/mbed-os-example-nfc that referenced this pull request Mar 17, 2020
@0xc0170
main: fix printf include
b9eec4e 
This PR ARMmbed/mbed-os#12581 failed due to removing some header files that could affect this.

Failures are
```
Compile [ 99.1%]: main.cpp
[Error] main.cpp@52,13: use of undeclared identifier 'printf'
[Error] main.cpp@64,13: use of undeclared identifier 'printf'
[Error] main.cpp@66,13: use of undeclared identifier 'printf'
[Error] main.cpp@74,13: use of undeclared identifier 'printf'
[Error] main.cpp@76,13: use of undeclared identifier 'printf'
[Error] main.cpp@81,9: use of undeclared identifier 'printf'
[Error] main.cpp@85,9: use of undeclared identifier 'printf'
```
@0xc0170 0xc0170 mentioned this pull request Mar 17, 2020
Merged
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 17, 2020

I sent PR fixing the include, will restart CI once integrated

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 25, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Mar 25, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added needs: work and removed needs: CI labels Mar 25, 2020
@adbridge
Copy link
Contributor

Looks like a CI issue, restarting

@mbed-ci
Copy link

mbed-ci commented Mar 27, 2020

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mbed-ci
Copy link

mbed-ci commented Mar 28, 2020

Test run: FAILED

Summary: 2 of 6 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-pytest
  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 30, 2020

I restarted client, will restarted test later

@mergify mergify bot added the needs: work label Mar 31, 2020
@mergify
Copy link

mergify bot commented Mar 31, 2020

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

@mergify mergify bot removed the needs: CI label Mar 31, 2020
@adbridge
Copy link
Contributor

@kjbracey-arm looks like this needs a rebase

@kjbracey
Fully enforce NonCopyable
874e36e 
Make NonCopyable fully operational so it gives compile errors in all
build profiles.
@kjbracey
Copy link
Contributor Author

Rebased.

@mergify mergify bot dismissed bulislaw’s stale review March 31, 2020 16:02

Pull request has been modified.

@mbed-ci
Copy link

mbed-ci commented Apr 2, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 5
Build artifacts

@0xc0170 0xc0170 requested review from bulislaw and evedon April 2, 2020 09:51
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 6, 2020

@bulislaw @evedon Please review

@mergify mergify bot added needs: CI and removed needs: review labels Apr 7, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 8, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Apr 8, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 6
Build artifacts

@0xc0170 0xc0170 merged commit d089c86 into ARMmbed:master Apr 9, 2020
@mergify mergify bot removed the ready for merge label Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@evedon evedon evedon approved these changes

@bulislaw bulislaw Awaiting requested review from bulislaw

Assignees
No one assigned
Labels
BREAKING-CHANGE release-version: 6.0.0-beta-1
Projects
None yet
Milestone
No milestone
Development

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