Re-add Shared Pointer Class into platform features #7815

donatieng · 2018-08-17T11:24:46Z

Description

This PR re-adds a shared pointer class to mbed OS (similar to the std::shared_ptr class introduced in C++11). This class (or a variation of) is used by different projects and used to be part of the "core-util" classes. The only change from this previous revision is the name: SharedPtr (to avoid any clashes with existing projects and be consistent with the SingletonPtr class).

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Breaking change

pan- · 2018-08-17T11:29:33Z

platform/SharedPtr.h

+
+            // increment new counter
+            if (counter) {
+                (*counter)++;


Could you use an atomic so we get the same guarantee as the regular shared_ptr ?

Hadn't realised it wasn't done atomically, will update the implementation :)

pan- · 2018-08-17T11:38:35Z

platform/SharedPtr.h

+  *
+  * Similar to std::shared_ptr in C++11.
+  *
+  * Usage: SharedPtr<class> POINTER(new class())


Please mention that it is not a full feature shared_ptr:

no weak ptr

no make_shared

no custom deleter

no cast operator

no enable_shared_from_this

Good point!

cmonr · 2018-08-17T15:59:55Z

platform/SharedPtr.h

+  *
+  * When POINTER is passed around by value the copy constructor and
+  * destructor counts the number of references to the original object.
+  * If the counter reaches zero, delete is called on the object pointed to.


Strange text?

Actually, it looks like many more 'L's were accidentally capitalized.

donatieng · 2018-08-17T18:06:05Z

I've cleaned up the class and its doc.

geky

Looks good to me 👍

I just had a bunch of nits

geky · 2018-08-17T18:09:34Z

platform/SharedPtr.h

+    SharedPtr(T* ptr): _ptr(ptr), _counter(NULL) {
+        // allocate counter on the heap so it can be shared
+        if(_ptr != NULL) {
+            _counter = (uint32_t*) malloc(sizeof(uint32_t));


Should these be new uint32_t so OOM asserts?

geky · 2018-08-17T18:15:03Z

platform/SharedPtr.h

+            if (_ptr != NULL) {
+                core_util_critical_section_enter();
+                (*_counter)++;
+                core_util_critical_section_exit();


Hmm, is core_util_atomic_incr_u32 applicable here?

Not a blocker, adopting atomic increments could be an optimization for later

geky · 2018-08-17T18:16:44Z

platform/SharedPtr.h

+    void decrement_counter() {
+        if (_counter) {
+            core_util_critical_section_enter();
+            if (*_counter == 1) {


Tiny tiny tiny micro-optimization, so feel free to ignore. But comparing with zero is slightly better in terms of code size. Once you hit zero there's also no race conditions possible.

donatieng · 2018-08-17T19:12:40Z

Thanks @geky - should be a bit more optimised now :)

geky

Looks great 👍

SenRamakri

@donatieng - Good work. Should any tests be added for SharedPtr class?

0xc0170 · 2018-08-20T09:46:15Z

Should any tests be added for SharedPtr class?

Definitely. And documentation (I'll add docs team to review).

0xc0170

Having Shared pointer +1

Can you check astyle travis job and fix the styling?

Astyle updated, waiting for tests

AnotherButler · 2018-08-20T18:51:42Z

Is there any reason this API shouldn't have a user API page added to the handbook?
Update: Edited for clarity

Copy edit for consistent capitalization and minor grammar nits.

donatieng · 2018-08-21T10:55:05Z

@AnotherButler created this PR for the handbook: ARMmbed/mbed-os-5-docs#671

0xc0170 · 2018-08-21T11:43:07Z

platform/SharedPtr.h

+    {
+        if (_ptr != NULL) {
+            core_util_critical_section_enter();
+            return *_counter;


you return prior exiting from critical section (is there an intention?)

donatieng · 2018-08-21T12:03:24Z

Urgh! Thanks for spotting this @0xc0170!

donatieng · 2018-08-21T17:33:49Z

LittleFS test failure seems quite unrelated to this PR's content

cmonr · 2018-08-21T17:40:06Z

@donatieng Correct. We found something external that appears to have affected Mbed OS CI. Breaking change has been reverted, and Travis CI jobs are being restarted.

donatieng · 2018-08-22T10:00:37Z

Thanks @cmonr - unless anyone has any objections, I think this one is good for morphing :)

0xc0170 · 2018-08-22T11:55:42Z

/morph build

0xc0170 · 2018-08-22T12:11:10Z

@studavekar @cmonr IllegalStateException again the last build run

/morph build

mbed-ci · 2018-08-22T13:05:22Z

Build : SUCCESS

Build number : 2862
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7815/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-08-22T13:47:50Z

Build : SUCCESS

Build number : 2863
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7815/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-08-22T15:10:13Z

Exporter Build : SUCCESS

Build number : 2492
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7815/

mbed-ci · 2018-08-22T15:50:18Z

Exporter Build : FAILURE

Build number : 2493
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7815/

mbed-ci · 2018-08-23T13:15:47Z

Test : SUCCESS

Build number : 2627
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7815/2627

0xc0170 · 2018-08-23T13:28:41Z

/morph export-build

mbed-ci · 2018-08-23T14:52:14Z

Test : SUCCESS

Build number : 2628
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7815/2628

mbed-ci · 2018-08-23T14:57:50Z

Exporter Build : SUCCESS

Build number : 2501
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7815/

donatieng requested review from geky and a team August 17, 2018 11:25

Re-introduce a shared pointer class in mbed OS

9ebfbc0

donatieng force-pushed the shared_ptr branch from 36c442e to 9ebfbc0 Compare August 17, 2018 11:31

pan- reviewed Aug 17, 2018

View reviewed changes

cmonr added the needs: review label Aug 17, 2018

cmonr reviewed Aug 17, 2018

View reviewed changes

cmonr requested a review from a team August 17, 2018 16:02

cmonr added needs: work and removed needs: review labels Aug 17, 2018

Donatien Garnier added 3 commits August 17, 2018 18:25

Make shared pointer thread safe and clean-up class and doc

f45642c

Cleanup shared pointer implementation and add reset() methods

2608478

Cleanup description

ac79b00

cmonr added the needs: review label Aug 17, 2018

geky approved these changes Aug 17, 2018

View reviewed changes

donatieng mentioned this pull request Aug 17, 2018

Add initial NFC support to Mbed OS #7822

Merged

Optimize SharedPtr code

6f3c07f

geky approved these changes Aug 17, 2018

View reviewed changes

SenRamakri reviewed Aug 17, 2018

View reviewed changes

cmonr removed the needs: work label Aug 17, 2018

0xc0170 requested a review from AnotherButler August 20, 2018 09:46

0xc0170 previously requested changes Aug 20, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Aug 20, 2018

Update formatting for SharedPtr.h

d9c1748

Add Greentea tests for shared pointer

3c599fe

Copy edit SharedPtr.h

7086d69

Copy edit for consistent capitalization and minor grammar nits.

bulislaw approved these changes Aug 21, 2018

View reviewed changes

donatieng mentioned this pull request Aug 21, 2018

Add documentation for shared pointer class ARMmbed/mbed-os-5-docs#671

Merged

0xc0170 requested changes Aug 21, 2018

View reviewed changes

Fix use_count() in SharedPtr

298f847

0xc0170 approved these changes Aug 21, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Aug 22, 2018

0xc0170 added the release-version: 5.10.0-rc1 label Aug 22, 2018

cmonr mentioned this pull request Aug 23, 2018

Updating SPIFBlockDevice to latest ARMmbed/mbed-os-example-blockdevice#6

Merged

cmonr merged commit deb905d into ARMmbed:master Aug 23, 2018

0xc0170 removed the needs: CI label Aug 23, 2018

Re-add Shared Pointer Class into platform features #7815

Re-add Shared Pointer Class into platform features #7815

Uh oh!

Conversation

donatieng commented Aug 17, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng commented Aug 17, 2018

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng commented Aug 17, 2018

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

SenRamakri left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 20, 2018

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

AnotherButler commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donatieng commented Aug 21, 2018

Uh oh!

0xc0170 Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donatieng commented Aug 21, 2018

Uh oh!

cmonr commented Aug 21, 2018

Uh oh!

donatieng commented Aug 22, 2018

Uh oh!

0xc0170 commented Aug 22, 2018

Uh oh!

0xc0170 commented Aug 22, 2018

Uh oh!

mbed-ci commented Aug 22, 2018

Triggering tests

Uh oh!

mbed-ci commented Aug 22, 2018

Triggering tests

Uh oh!

mbed-ci commented Aug 22, 2018

Uh oh!

mbed-ci commented Aug 22, 2018

Uh oh!

mbed-ci commented Aug 23, 2018

Uh oh!

cmonr Aug 17, 2018 •

edited

Loading

AnotherButler commented Aug 20, 2018 •

edited

Loading

0xc0170 Aug 21, 2018 •

edited

Loading

donatieng commented Aug 21, 2018 •

edited

Loading