Give an option to remove 'using namespace' #7760

deepikabhavnani · 2018-08-10T20:15:44Z

Description

~~DO NOT MERGE~~

I have created this PR to get input from all teams for solution to namespace issue.
Purpose:

To give an option to remove "using namespace" with MBED_NO_NAMESPACE macro
Move all classes in Mbed OS inside mbed namespace. Addition of existing classes inside mbed namespace breaks compatibility, hence below code should be added to maintain compatibility.

// Added "using" for backwards compatibility
#ifndef MBED_NO_NAMESPACE
using mbed::<Classname>;
#endif

List of all classes to be moved to mbed namespace. Feel free to add to the list or comment below if you know of any class additions.

[ ] All Block Devices
[ ] Filesystem's

Related: #6684

Pull request type

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

CC @geky @bridadan @dlfryar @0xc0170 @pan- @kjbracey-arm

kjbracey · 2018-08-13T09:13:00Z

Looks conceptually good to me. Is the intent then that application code would either define this MBED_NO_NAMESPACE globally or on a per-file basis before including mbed header files?

Little bit doubtful about the name - it's more MBED_NO_USING, but then that sounds weird. MBED_NO_GLOBAL_NAMESPACE?

There is another possible alternative approach. You could do what the C++ library does and make it so that #include <mbed> doesn't do the using, but #include <mbed.h> does.

That is a pattern I've not seen outside the standard C++ library, but given that it's there it shouldn't be totally unfamiliar. But it doesn't match our use of .h for all other header files, and not sure what other problems lack of .h would cause.

Maybe #include of some other name that omits the using - "mbed_os.h"? That approach doesn't cover backwards compatibility with people directly including ObservingBlockDevice.h though.

pan- · 2018-08-13T10:27:09Z

There is another possible alternative approach. You could do what the C++ library does and make it so that #include doesn't do the using, but #include <mbed.h> does.

I like the idea but I fear it confuses some of our developers. Our codebase itself is not consistent with the practice.

For the name, like @kjbracey-arm I think it should be changed. MBED_NO_NAMESPACE feels like it removes namespace. I can think of few names: MBED_NO_GLOBAL_USING_DIRECTIVE or MBED_NO_GLOBAL_USING_NAMESPACE.

What's the path going forward ? Is there a plan to set this macro by default ? It may be worthwhile to think of how components are mapped into namespaces as we may require more than just the mbed namespace.

deepikabhavnani · 2018-08-13T14:11:38Z

Is the intent then that application code would either define this MBED_NO_NAMESPACE globally or on a per-file basis before including mbed header files?

Can leave that to application. It should work both ways, if we make sure mbed-os compiles with flag enabled globally. Default we cannot enable it, but adding it to CI or not will be question.

For the name, like @kjbracey-arm I think it should be changed. MBED_NO_NAMESPACE feels like it removes namespace. I can think of few names:MBED_NO_GLOBAL_USING_DIRECTIVE or MBED_NO_GLOBAL_USING_NAMESPACE.

Yes , good names are appreciated. MBED_NO_GLOBAL_USING_NAMESPACE / MBED_NO_GLOBAL_USING_DIRECTIVE 👍

#include doesn't do the using, but #include <mbed.h> does.

Agree with @pan- it will be confusing, Also as you mentioned it will not be backward compatible we should avoid that solution.

bmcdonnell-ionx · 2018-08-21T19:29:24Z

As a user, I agree with not dropping the .h extension from your header file names. AFAIK it's not a convention outside the C++ library.

Default we cannot enable it

How about for Mbed OS 6.0?

Or, zooming out, why is mbed.h necessary or desirable at all? For instance, you don't #include <TheWholeCppLibrary.h>; you #include just the headers you need. Why is it a good thing to have a single header that makes the entire OS/lib interface visible?

@pan- said previously (#5679 (comment)):

From my perspective, the issue in that case is the use of mbed.h directly in os/production code. It is not designed for that usage and unsafe regards to name collision by design. The sole and only viable solution is to not use it in code that needs to be name collision free.

Unfortunately uses of mbed.h are not properly documented which lead developers to use it in the wrong context. That would be nice to have a clear documentation bloc explaining what mbed.h is as well as how and where it should be used.

About the changes proposed, I personally believe that even if mbed-os.h solves the name collision issue it is not a facility that should be used in production code except if we want to encourage build time increases.

kjbracey · 2018-08-22T10:41:54Z

Or, zooming out, why is mbed.h necessary or desirable at all?

My understanding is that it's desirable for "simplicity" - simple apps just include "mbed.h". Same basic argument as putting every single folder on the include path, and putting everything in the global namespace.

Also, possibly some of the core built-in stuff doesn't have as obviously clear "what to include" rules as "Thread.h" for Thread, so I guess it mops up a bunch of miscellaneous stuff - types, error codes etc, giving us leeway to restructure a bit.

I would say it should not be used in any OS or library header files, so that the name pollution isn't forced on application code.

0xc0170 · 2018-08-22T11:34:41Z

events/mbed_shared_queues.cpp

@@ -15,7 +15,10 @@
 */

 #include "events/mbed_shared_queues.h"
-#include "mbed.h"
+
+#ifdef MBED_CONF_RTOS_PRESENT


I can see similar changes in this PR that could be separated from adding new config (changes in events files at least)? As I expect with this config change to touch only mbed or std namespace (add it ,remove it).

Removing namespace i.e building mbed-os with "-DMBED_NO_GLOBAL_USING_DIRECTIVE" results in build failures as 'mbed.h' added here will not add RTOS / Mbed namespace in cpp files as well.

This cleanup "i.e. add only required namespace/header file" is needed for removal of namespace to work. I can add it as separate PR, and make this PR to be dependent on that.

bmcdonnell-ionx · 2018-08-22T16:24:59Z

@kjbracey-arm,

I would say it should not be used in any OS or library header files, so that the name pollution isn't forced on application code.

I absolutely agree.

Assuming you do keep it, please also document in appropriate places (when it becomes true) that application code doesn't need to #include "mbed.h".

deepikabhavnani · 2018-08-22T19:32:03Z

I would say it should not be used in any OS or library header files, so that the name pollution isn't forced on application code.

👍 Removing mbed.h here - #7864

deepikabhavnani · 2018-10-11T17:56:06Z

Dependent on #7864 #8002 #8008

bulislaw

+1

0xc0170 · 2018-11-26T10:36:44Z

@deepikabhavnani All storage PR landed, or still waiting for some here?

deepikabhavnani · 2018-11-26T13:03:18Z

@deepikabhavnani All storage PR landed, or still waiting for some here?

Review from storage team is pending as well

dannybenor · 2018-11-26T13:42:55Z

@offirko can you please review?

offirko

no remarks, LGTM.

bulislaw · 2018-11-26T14:54:32Z

All the PRs need to be engineering ready (marked as "needs: CI") by the end of the day (Austin time). Otherwise it won't make 5.11 and will need to come in the next release (5.12 for features, 5.11.1 for fixes and new platforms).

…mespace Macro guard `MBED_NO_GLOBAL_USING_DIRECTIVE` is added around namespace, to avoid polluting users namespace.

deepikabhavnani · 2018-11-26T15:30:55Z

Rebased

cmonr · 2018-11-26T17:14:12Z

CI started.

Adding new modules inside the namespace could be breaking change for existing code base hence add `using namespace::class` for classes newly added to mbed namespace to maintian backwards compatibility. MBED_NO_GLOBAL_USING_DIRECTIVE is added to remove auto-addition of namespace Macro guard `MBED_NO_GLOBAL_USING_DIRECTIVE` is added around namespace, to avoid polluting users namespace.

deepikabhavnani · 2018-11-26T19:36:45Z

@cmonr - Fixed for failing device. Needs CI restart

cmonr · 2018-11-27T02:09:08Z

#7760 (comment)

I like it. So fast, didn't even realize this had a problem.

CI restarted.

mbed-ci · 2018-11-27T06:27:03Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 10
Build artifacts
Build logs

cmonr added the do not merge label Aug 10, 2018

cmonr added the needs: review label Aug 13, 2018

cmonr requested review from pan-, dlfryar-zz, geky, 0xc0170, bridadan and kjbracey August 13, 2018 14:45

geky mentioned this pull request Aug 14, 2018

Add default block device support (SD, SPIF and FLASHIAP) #7774

Merged

geky mentioned this pull request Aug 21, 2018

Extending SPIF BD SFDP support ARMmbed/spif-driver#20

Merged

deepikabhavnani force-pushed the feature_namespace branch from cde3907 to 3192560 Compare August 21, 2018 21:52

0xc0170 reviewed Aug 22, 2018

View reviewed changes

deepikabhavnani mentioned this pull request Aug 22, 2018

Add required header file and namespace element instead add all. #7864

Merged

deepikabhavnani force-pushed the feature_namespace branch 2 times, most recently from ced54da to 7765de7 Compare August 22, 2018 19:10

deepikabhavnani added the needs: preceding PR label Aug 27, 2018

This was referenced Sep 5, 2018

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

Merged

SD - Add required header file and namespace element instead add all #8006

Merged

Add required header file and namespace element instead add all #8008

Merged

deepikabhavnani mentioned this pull request Oct 4, 2018

Platform: fix missing namespace for SharedPtr #8329

Merged

cmonr added needs: work and removed do not merge labels Oct 12, 2018

bulislaw approved these changes Nov 19, 2018

View reviewed changes

0xc0170 added needs: preceding PR needs: work and removed needs: review labels Nov 19, 2018

SenRamakri approved these changes Nov 23, 2018

View reviewed changes

offirko approved these changes Nov 26, 2018

View reviewed changes

MBED_NO_GLOBAL_USING_DIRECTIVE is added to remove auto-addition of na…

b6e381b

…mespace Macro guard `MBED_NO_GLOBAL_USING_DIRECTIVE` is added around namespace, to avoid polluting users namespace.

deepikabhavnani force-pushed the feature_namespace branch from 26a3d12 to d54e682 Compare November 26, 2018 15:30

deepikabhavnani added needs: CI and removed needs: preceding PR needs: work labels Nov 26, 2018

cmonr added the risk: G label Nov 26, 2018

deepikabhavnani force-pushed the feature_namespace branch from d54e682 to da69da9 Compare November 26, 2018 19:36

NirSonnenschein added ready for merge and removed needs: CI labels Nov 27, 2018

0xc0170 merged commit 436644b into master Nov 27, 2018

0xc0170 removed the ready for merge label Nov 27, 2018

0xc0170 deleted the feature_namespace branch November 27, 2018 08:07

This was referenced Nov 27, 2018

header files should not have using statements (namespaces) #5679

Closed

BlockDevice classes should be moved to mbed namespace #6684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give an option to remove 'using namespace' #7760

Give an option to remove 'using namespace' #7760

deepikabhavnani commented Aug 10, 2018 •

edited

Loading

kjbracey commented Aug 13, 2018 •

edited

Loading

pan- commented Aug 13, 2018 •

edited

Loading

deepikabhavnani commented Aug 13, 2018 •

edited

Loading

bmcdonnell-ionx commented Aug 21, 2018

kjbracey commented Aug 22, 2018

0xc0170 Aug 22, 2018

deepikabhavnani Aug 22, 2018

bmcdonnell-ionx commented Aug 22, 2018

deepikabhavnani commented Aug 22, 2018

deepikabhavnani commented Oct 11, 2018

bulislaw left a comment

0xc0170 commented Nov 26, 2018

deepikabhavnani commented Nov 26, 2018

dannybenor commented Nov 26, 2018

offirko left a comment

bulislaw commented Nov 26, 2018

deepikabhavnani commented Nov 26, 2018

cmonr commented Nov 26, 2018

deepikabhavnani commented Nov 26, 2018

cmonr commented Nov 27, 2018

mbed-ci commented Nov 27, 2018

Give an option to remove 'using namespace' #7760

Give an option to remove 'using namespace' #7760

Conversation

deepikabhavnani commented Aug 10, 2018 • edited Loading

Description

Pull request type

kjbracey commented Aug 13, 2018 • edited Loading

pan- commented Aug 13, 2018 • edited Loading

deepikabhavnani commented Aug 13, 2018 • edited Loading

bmcdonnell-ionx commented Aug 21, 2018

kjbracey commented Aug 22, 2018

0xc0170 Aug 22, 2018

Choose a reason for hiding this comment

deepikabhavnani Aug 22, 2018

Choose a reason for hiding this comment

bmcdonnell-ionx commented Aug 22, 2018

deepikabhavnani commented Aug 22, 2018

deepikabhavnani commented Oct 11, 2018

bulislaw left a comment

Choose a reason for hiding this comment

0xc0170 commented Nov 26, 2018

deepikabhavnani commented Nov 26, 2018

dannybenor commented Nov 26, 2018

offirko left a comment

Choose a reason for hiding this comment

bulislaw commented Nov 26, 2018

deepikabhavnani commented Nov 26, 2018

cmonr commented Nov 26, 2018

deepikabhavnani commented Nov 26, 2018

cmonr commented Nov 27, 2018

mbed-ci commented Nov 27, 2018

Test run: SUCCESS

deepikabhavnani commented Aug 10, 2018 •

edited

Loading

kjbracey commented Aug 13, 2018 •

edited

Loading

pan- commented Aug 13, 2018 •

edited

Loading

deepikabhavnani commented Aug 13, 2018 •

edited

Loading