Skip to content

Storage: Add required header file and namespace element instead add all #8002

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
Oct 12, 2018
Merged

Storage: Add required header file and namespace element instead add all #8002

merged 1 commit into from
Oct 12, 2018

Conversation

deepikabhavnani
Copy link

Description

Source inside mbed-os should not be using "mbed.h" even in CPP files, instead required header file and namespace should be explicitly added inside mbed-os

With #7760 PR, we will give an option to remove namespace.

Pull request type

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

@cmonr cmonr requested review from kjbracey and a team September 5, 2018 15:47
@deepikabhavnani
Copy link
Author

@ARMmbed/mbed-os-storage - Please review

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 1, 2018

Needs a rebase to resolve a conflict. We should be good for Ci afterwards.

@deepikabhavnani
Copy link
Author

Rebased to resolve conflicts

@deepikabhavnani
Copy link
Author

@ARMmbed/mbed-os-storage - Please review

yossi2le
yossi2le reviewed Oct 7, 2018
View reviewed changes
features/storage/blockdevice/ChainingBlockDevice.h Outdated
#include "mbed.h"

#include "platform/mbed_assert.h"
#include "ChainingBlockDevice.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does ChainingBlockDevice.h include itself?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch.. 👍

yossi2le
yossi2le reviewed Oct 7, 2018
View reviewed changes
features/storage/blockdevice/ObservingBlockDevice.cpp
@@ -36,7 +35,7 @@ ObservingBlockDevice::~ObservingBlockDevice()
// Does nothing
}

void ObservingBlockDevice::attach(Callback<void(BlockDevice *)> cb)
void ObservingBlockDevice::attach(mbed::Callback<void(BlockDevice *)> cb)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not adding using namespace mbed?

Copy link
Author

Choose a reason for hiding this comment

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

Consistency, also after this cleanup next PR is to add all block devices and filesystem into the mbed namespace

Copy link
Contributor

@yossi2le yossi2le Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I can live with it. Specifically, if next PR is to bring those block devices into mbed namespace.

Copy link
Author

Choose a reason for hiding this comment

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

I have that PR already in place, but cannot merge unless this cleanup is done

yossi2le
yossi2le suggested changes Oct 7, 2018
View reviewed changes
Copy link
Contributor

@yossi2le yossi2le left a comment

Choose a reason for hiding this comment

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

I think in general it looks good besides 2 minor issues.

features/storage/nvstore/TESTS/nvstore/functionality/main.cpp
@@ -510,7 +510,7 @@ static void nvstore_multi_thread_test()
if (!threads[i]) {
goto mem_fail;
}
threads[i]->start(callback(thread_test_worker));
threads[i]->start(mbed::callback(thread_test_worker));
Copy link
Contributor

Choose a reason for hiding this comment

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

Again why not just adding using namespace mbed?

Copy link
Author

Choose a reason for hiding this comment

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

I tried to be consistent with the code in file https://github.com/ARMmbed/mbed-os/pull/8002/files/adc255fbec2e56c9058811a590f301d4d255aeaf#diff-3c5c355089a195140705371a70b7b5d0L464

Namespace is not used for std / rtos..

Copy link
Contributor

@yossi2le yossi2le Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Do you intend to bring nvstore files also into mbed namespace in the next PR?

Copy link
Author

Choose a reason for hiding this comment

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

Block device and file system was for sure.. NVstore will have to check with the storage and other teams

@cmonr
Copy link
Contributor

cmonr commented Oct 11, 2018

/moprh build

@cmonr
Copy link
Contributor

cmonr commented Oct 11, 2018

Whoops.

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 11, 2018

Build : SUCCESS

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

Triggering tests

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

This was referenced Oct 11, 2018
Merged
Merged
@mbed-ci
Copy link

mbed-ci commented Oct 11, 2018

Exporter Build : SUCCESS

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

@mbed-ci
Copy link

mbed-ci commented Oct 11, 2018

Test : SUCCESS

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

@adbridge adbridge merged commit 8dcc949 into ARMmbed:master Oct 12, 2018
@deepikabhavnani deepikabhavnani deleted the storage_remove_mbed_h branch October 15, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pan- pan- pan- approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@kjbracey kjbracey kjbracey approved these changes

@SenRamakri SenRamakri SenRamakri approved these changes

@yossi2le yossi2le yossi2le approved these changes

Assignees
No one assigned
Labels
release-version: 5.10.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@deepikabhavnani @0xc0170 @cmonr @mbed-ci @pan- @kjbracey @SenRamakri @yossi2le @adbridge @studavekar