Skip to content

BlockDevice classes should be moved to mbed namespace #6684

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

Closed
AGlass0fMilk opened this issue Apr 19, 2018 · 16 comments
Closed

BlockDevice classes should be moved to mbed namespace #6684

AGlass0fMilk opened this issue Apr 19, 2018 · 16 comments

Comments

@AGlass0fMilk
Copy link
Member

BlockDevice classes (and all mbed features) should be moved to the mbed namespace and should not #include "mbed.h"

@bmcdonnell-ionx
Copy link
Contributor

Related: #5679. 😃

@AGlass0fMilk
Copy link
Member Author

Sorta related: #6421 and PR #6427 😃

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 20, 2018

BlockDevice classes (and all mbed features) should be moved to the mbed namespace and should not #include "mbed.h"

👍

@geky can you please review

@geky
Copy link
Contributor

geky commented Apr 20, 2018

Yep! 👍

@geky
Copy link
Contributor

geky commented Apr 20, 2018

Do you think we could make a CI task for this? Check that any header file with camel case name does not have "include mbed.h" and has "namespace mbed"?

@bmcdonnell-ionx
Copy link
Contributor

@geky, good idea...

Is that better discussed here, or on #5679?

has "namespace mbed"

More precisely, maybe something more like ^ *namespace mbed.

And check that all header files (regardless of CamelCase) do not have "using *namespace".

@deepikabhavnani
Copy link

CC @ARMmbed/mbed-os-storage

@geky
Copy link
Contributor

geky commented Aug 10, 2018

Reopening since #7663 is being reverted by #7757

@geky geky reopened this Aug 10, 2018
@geky
Copy link
Contributor

geky commented Aug 10, 2018

@deepikabhavnani @bridadan @0xc0170 @pan- @kjbracey-arm

Here's an idea:

  1. Stealing @kjbracey-arm's idea, lets add an MBED_NO_POLLUTION macro that disables the "using namespace mbed" declarations.

    By itself this doesn't solve the BlockDevice -> mbed::BlockDevice problem, but it's a step in the right direction and allows users to opt-out of the namespace pollution.

    So for example in storage-selector:

    #define MBED_NO_POLLUTION
    #include "mbed.h"
    #undef MBED_NO_POLLUTION
    
    // use namespaced names
    void doit(mbed::BlockDevice *bd);
  2. Then, we can use this macro in BlockDevice to keep the existing behaviour unless the includer opts out.

    By adding this to the end of @deepikabhavnani's BlockDevice.h:

    #ifndef MBED_NO_POLLUTION
    using mbed::BlockDevice;
    #endif

    This way we keep backwards compatibility:

    #include "BlockDevice.h"
    
    // use non-namespaced names
    void doit(BlockDevice *bd);

    But also allow you to opt-out and use the namespaced BlockDevice in new code:

    #ifndef MBED_NO_POLLUTION
    #include "mbed.h"
    #include "BlockDevice.h"
    #endif
    
    // use non-namespaced names
    void doit(mbed::BlockDevice *bd);

@geky
Copy link
Contributor

geky commented Aug 10, 2018

However, there is one problem, once we introduce MBED_NO_POLLUTION, we can't move more classes into the mbed namespace without breaking compatibility again.

  1. One option is to mark MBED_NO_POLLUTION as expiremental:

    #ifdef MBED_NO_POLLUTION
    #warn "MBED_NO_POLLUTION support is currently expiremental. Classes may move into the mbed namespace as clean up continues. If you use MBED_NO_POLLUTION make sure you are not using any classes without the mbed:: prefix, as these may change in the future. Feel free to raise an issue on https://github.com/armmbed/mbed-os if you find any classes outside of the mbed namespace."
    using namespace mbed;
    #endif
  2. Another option is to provide "stages". We start with MBED_NO_POLLUTION_STAGE_1, and every time we move classes into the mbed namespace we add another MBED_NO_POLLUTION_STAGE_2. Once we are confident all classe are in the mbed namespace (CI check?) we could then introduce MBED_NO_POLLUTION.This would insure compatibility, but would be more noisy for users.

@deepikabhavnani
Copy link

deepikabhavnani commented Aug 10, 2018

@geky - Can we introduce MBED_NO_POLLUTION, when we are confident that all classes are in mbed namespace. And till we do that every existing class added to namespace should explicitly add 'using mbed::;` in header file.

This approach will help us to move classes into mbed namespace though still polluted, but will have way to un-pollute in one go.

@bmcdonnell-ionx
Copy link
Contributor

How far off is Mbed OS 6.0? Maybe you could just make the breaking changes you need to make then, including

  1. Removing all using namespace statements from all header files - including and especially mbed.h. (header files should not have using statements (namespaces) #5679)
  2. Removing all #include "mbed.h" statements from all Mbed OS code. Only #include what you need in each file, and no more.
  3. Document the intended use cases for mbed.h. I think that would be (1) building Mbed OS as a library, and (2) ease of use / "quick start" for users in application code.

Re: 3, when an app developer is including the whole Mbed OS source tree under a project (per the current paradigm), it would be preferable to auto-detect and build only the parts of Mbed OS which are needed by the application.

@AGlass0fMilk
Copy link
Member Author

@bmcdonnell-ionx Regarding the 3rd case:
It would be nice to be able to only have one copy of mbed-os and build multiple programs (main app, bootloader, etc) from that to maintain codebase consistency in an organization. The mbed build system works, but I have definitely had some pains when using it. Maybe a .json file where you can specify additional include paths? This is a separate issue though...

@bmcdonnell-ionx
Copy link
Contributor

bmcdonnell-ionx commented Aug 10, 2018

It would be nice to be able to only have one copy of mbed-os and build multiple programs (main app, bootloader, etc) from that to maintain codebase consistency in an organization.

I've asked for that elsewhere, too. But as long as the OS source is under the app source, assuming no "library build" option is specified, I think the the minimal build would be optimal.

@adbridge
Copy link
Contributor

adbridge commented Oct 4, 2018

Internal Jira reference: https://jira.arm.com/browse/IOTSTOR-507

@deepikabhavnani
Copy link

deepikabhavnani commented Nov 29, 2018

#7760 should resolve this. Please reopen if required

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

No branches or pull requests

7 participants