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

Re-add Shared Pointer Class into platform features #7815

Merged
merged 9 commits into from Aug 23, 2018

Conversation

@donatieng
Member

donatieng commented Aug 17, 2018

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

@donatieng donatieng requested review from geky and ARMmbed/mbed-os-core Aug 17, 2018

@donatieng donatieng force-pushed the donatieng:shared_ptr branch from 36c442e to 9ebfbc0 Aug 17, 2018

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

This comment has been minimized.

@pan-

pan- Aug 17, 2018

Member

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

This comment has been minimized.

@donatieng

donatieng Aug 17, 2018

Member

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

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

This comment has been minimized.

@pan-

pan- Aug 17, 2018

Member

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

This comment has been minimized.

@donatieng

donatieng Aug 17, 2018

Member

Good point!

@cmonr cmonr added the needs: review label Aug 17, 2018

*
* 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.

This comment has been minimized.

@cmonr

cmonr Aug 17, 2018

Contributor

Strange text?

This comment has been minimized.

@cmonr

cmonr Aug 17, 2018

Contributor

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

@cmonr cmonr requested a review from ARMmbed/mbed-os-core Aug 17, 2018

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

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 17, 2018

I've cleaned up the class and its doc.

@cmonr cmonr added the needs: review label Aug 17, 2018

@geky

geky approved these changes Aug 17, 2018

Looks good to me 👍

I just had a bunch of nits

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));

This comment has been minimized.

@geky

geky Aug 17, 2018

Member

Should these be new uint32_t so OOM asserts?

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

This comment has been minimized.

@geky

geky Aug 17, 2018

Member

Hmm, is core_util_atomic_incr_u32 applicable here?

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

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

This comment has been minimized.

@geky

geky Aug 17, 2018

Member

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

This comment has been minimized.

Member

donatieng commented Aug 17, 2018

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

@geky

geky approved these changes Aug 17, 2018

Looks great 👍

@SenRamakri

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

@cmonr cmonr removed the needs: work label Aug 17, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 20, 2018

Should any tests be added for SharedPtr class?

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

@0xc0170 0xc0170 requested a review from AnotherButler Aug 20, 2018

@0xc0170

Having Shared pointer +1

Can you check astyle travis job and fix the styling?

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

Astyle updated, waiting for tests

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Aug 20, 2018

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

Copy edit SharedPtr.h
Copy edit for consistent capitalization and minor grammar nits.
@donatieng

This comment has been minimized.

Member

donatieng commented Aug 21, 2018

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

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

This comment has been minimized.

@0xc0170

0xc0170 Aug 21, 2018

Member

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

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 21, 2018

Urgh! Thanks for spotting this @0xc0170!

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 21, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 21, 2018

@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

This comment has been minimized.

Member

donatieng commented Aug 22, 2018

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

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

@studavekar @cmonr IllegalStateException again the last build run

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 22, 2018

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

This comment has been minimized.

mbed-ci commented Aug 22, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 22, 2018

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 23, 2018

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

15 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.12%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 584 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9098 cycles (-1163 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

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

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 29, 2018

Merge pull request ARMmbed#7815 from donatieng/shared_ptr
Re-add Shared Pointer Class into platform features
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment