Skip to content

Platform: fix missing namespace for SharedPtr #8329

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

Merged
merged 3 commits into from
Oct 31, 2018

Conversation

paul-szczepanek-arm
Copy link
Member

Description

Add missing mbed namespace to class. We avoid making a breaking change with the "using".

Pull request type

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

@0xc0170 0xc0170 requested a review from a team October 4, 2018 15:27
@deepikabhavnani
Copy link

deepikabhavnani commented Oct 4, 2018

Please see #7760 and #6684. It will be good to add all namespace related fixes at one place.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 11, 2018

Please see #7760 and #6684. It will be good to add all namespace related fixes at one place.

@deepikabhavnani should this be cherry pick there or ?

@deepikabhavnani
Copy link

deepikabhavnani commented Oct 11, 2018

should this be cherry pick there or ?

Yes cherry pick will be good option, but will have to wait for header file fixes first

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 11, 2018

Yes cherry pick will be good option, but will have to wait for header file fixes first

What is the preceding PR from the earlier referenced PR?

@deepikabhavnani
Copy link

PR#7760 is dependent on #7864 #8002 #8008 for now. Few more can come in later.

In order to have namespace free mbed-os, we will have to remove "mbed.h" and "using" from all header files first. Pending in "features" folder - BLE/Cellular and all Networking

@cmonr
Copy link
Contributor

cmonr commented Oct 25, 2018

This is clear to proceed!

@@ -285,4 +287,8 @@ bool operator!= (U lhs, const SharedPtr<T> &rhs)
return ((T *) lhs != rhs.get());
}

} /* namespace mbed */

using mbed::SharedPtr;
Copy link

@deepikabhavnani deepikabhavnani Oct 25, 2018

Choose a reason for hiding this comment

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

Along with addition of namespace and backward compatibility, we need to make sure we do not corrupt the global namespace hence a small change needed:

using should be under #ifndef MBED_NO_GLOBAL_USING_DIRECTIVE macro.
https://github.com/ARMmbed/mbed-os/pull/7760/files#diff-3716425795ebc1692d5a64771fa4f35eR169

@paul-szczepanek-arm
Copy link
Member Author

paul-szczepanek-arm commented Oct 26, 2018

using directive is now wrapped in the macro, can you sign off the reivew please @deepikabhavnani

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Backward compatible

@0xc0170 0xc0170 requested a review from SenRamakri October 29, 2018 12:17
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 30, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 30, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Oct 30, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 30, 2018

@cmonr
Copy link
Contributor

cmonr commented Oct 30, 2018

IAR network license issues.
/morph mbed2-build

java.lang.NoClassDefFoundError: Could not initialize class hudson.slaves.SlaveComputer
Export CI had some issues, as well as IAR network issues.
/morph export-build

Aaaand finally, I don't think the lone test failue was due to the PR...
/morph test

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 30, 2018

NRF52_DK-ARM.tests-mbed_hal-rtc_time_conv.test make time and local time - RTC leap years partial support

This looks like it's on master and appears seldomly these days.

@mprse Can you please review NRF52_DK-ARM.tests-mbed_hal-rtc_time_conv.test make time and local time - RTC leap years partial support failure? I've seen it today already at least in another PR

@mbed-ci
Copy link

mbed-ci commented Oct 30, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2018

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

@mbed-ci
Copy link

mbed-ci commented Oct 31, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants