Skip to content

fix namespaces for MBED_NO_GLOBAL_USING_DIRECTIVE feature #13568

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
Closed
wants to merge 1 commit into from
Closed

fix namespaces for MBED_NO_GLOBAL_USING_DIRECTIVE feature #13568

wants to merge 1 commit into from

Conversation

EmbedEngineer
Copy link

Summary of changes

This pull request fixes issue #13539.

Atm when activating MBED_NO_GLOBAL_USING_DIRECTIVE feature, mbed is not compiling when activation the BLE feature, due to missing namespaces in various files.

Impact of changes

Migration actions required

Documentation

None

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 

[X] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 8, 2020
@ciarmcom
Copy link
Member

ciarmcom commented Sep 8, 2020

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

@ciarmcom ciarmcom requested review from a team September 8, 2020 08:00
@pan-
Copy link
Member

pan- commented Sep 8, 2020

@EmbedEngineer The BLE part should be addressed by #13475

@EmbedEngineer
Copy link
Author

@pan- OK so I will wait for #13475

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 9, 2020

I added a label "needs: preceding PR"

@mergify
Copy link

mergify bot commented Sep 9, 2020

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

@EmbedEngineer EmbedEngineer force-pushed the fix_namespaces_for_MBED_NO_GLOBAL_USING_DIRECTIVE_feature branch from 39da51a to 5cb15e8 Compare September 10, 2020 13:12
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 14, 2020

@pan- OK so I will wait for #13475

@EmbedEngineer was this rebased after this PR was merged - please provide an update

@EmbedEngineer EmbedEngineer force-pushed the fix_namespaces_for_MBED_NO_GLOBAL_USING_DIRECTIVE_feature branch from 5cb15e8 to 66c1a1f Compare September 21, 2020 11:51
@EmbedEngineer
Copy link
Author

@0xc0170 update is out

0xc0170
0xc0170 previously requested changes Oct 19, 2020
View reviewed changes
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.

LGTM just one styling request

drivers/source/usb/TaskBase.cpp Outdated
@@ -21,19 +21,19 @@
#include "rtos/Semaphore.h"
#include "platform/mbed_critical.h"

TaskBase::TaskBase(TaskQueue *q)
events::TaskBase::TaskBase(TaskQueue *q)
Copy link
Contributor

Choose a reason for hiding this comment

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

just styling, should this be rather entirely in namespace events { } rather than each method?

@mergify
Copy link

mergify bot commented Nov 18, 2020

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

evedon
evedon reviewed Nov 20, 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.

Change fine but I agree with @0xc0170 that TaskBase and PolledQueue could be entirely in namespace events { } rather than each method.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 3, 2020

@EmbedEngineer would you be able to send a new Pr with the proposed fix ? The Ci status were updated (Travis migration), so it would be the best to create new PR.

@EmbedEngineer EmbedEngineer force-pushed the fix_namespaces_for_MBED_NO_GLOBAL_USING_DIRECTIVE_feature branch from 66c1a1f to c91a9c6 Compare December 9, 2020 08:15
@mergify mergify bot dismissed 0xc0170’s stale review December 9, 2020 08:16

Pull request has been modified.

@EmbedEngineer EmbedEngineer force-pushed the fix_namespaces_for_MBED_NO_GLOBAL_USING_DIRECTIVE_feature branch from c91a9c6 to bc23157 Compare December 9, 2020 08:43
@EmbedEngineer
Copy link
Author

@0xc0170 updated the PR

0xc0170
0xc0170 requested changes Dec 9, 2020
View reviewed changes
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.

One styling change, otherwise LGTM. WE can start CI once updated

drivers/usb/source/PolledQueue.cpp
@@ -21,17 +21,17 @@
#include "platform/Callback.h"


PolledQueue::PolledQueue(mbed::Callback<void()> cb): _cb(cb)
events::PolledQueue::PolledQueue(mbed::Callback<void()> cb): _cb(cb)
Copy link
Contributor

Choose a reason for hiding this comment

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

also this one to be in events namespace

@mergify mergify bot added needs: work and removed needs: CI labels Dec 9, 2020
@evedon evedon mentioned this pull request Dec 11, 2020
Merged
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 5, 2021

@EmbedEngineer Let us know if you can update ,or we shall create a separate PR to take over so this can get in? There's already similar PR referenced above #14031. Probably we can take this one there.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 5, 2021

We are proceeding with 14031 (cherry picked this pr there). I'll close this now.

@0xc0170 0xc0170 closed this Jan 5, 2021
@mergify mergify bot removed needs: work release-type: patch Indentifies a PR as containing just a patch labels Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evedon evedon evedon left review comments

@0xc0170 0xc0170 0xc0170 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EmbedEngineer @ciarmcom @pan- @0xc0170 @evedon