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

bmcdonnell-ionx · 2017-12-08T23:40:14Z

Description

Type: Bug | Enhancement

Several mbed-os header files have using statements in them. Most notably in mbed.h:

using namespace mbed;
using namespace std;

Reason to enhance or problem with existing solution

There are plenty of online references explaining why having using statements in header files is bad practice. In brief, it pollutes the global namespace for your users.

Suggested enhancement

All using statements should be removed from all header files.

Pros

Don't pollute your users' namespaces, forcing them to change their module names or create namespaces to avoid name collisions with your stuff.

Also, this gives users the option of always having namespace prefixes on things if they want it - and having the compiler enforce that - if they find that useful, in whatever situations.

Cons

Granted, it is a painful change for users who relied on it, so a migration plan is appropriate. e.g. Maybe explain the change in the release notes in mbed-os 5.7 or 5.8 or 6.0 that these using statements have been removed, and users can add the statements to their source files if they wish to minimize code changes.

The text was updated successfully, but these errors were encountered:

0xc0170 · 2017-12-11T08:29:20Z

This has been discussed few times, you can find an answer here:

https://os.mbed.com/questions/5492/c-design-pattern-or-problem/

Several mbed-os header files have using statements in them

Only mbed.h should do. I did quick search, and can find few spots that we should look at, and possibly fix.

cc @sg-

bmcdonnell-ionx · 2017-12-11T14:36:17Z

Only mbed.h should do.

Do you mean that you agree with me that the using statements should be removed? I found them in several places (commit tag mbed-os-5.6.6):

me@pc /cygdrive/c/dev/mbed-os-example-fat-filesystem/mbed-os
$ grep -Rw ^" *using .*;" | grep -e \.h:
drivers/Serial.h:    using SerialBase::read;
drivers/Serial.h:    using SerialBase::write;
drivers/UARTSerial.h:    using FileHandle::readable;
drivers/UARTSerial.h:    using FileHandle::writable;
events/mbed_events.h:using namespace events;
features/filesystem/fat/FATFileSystem.h:using namespace mbed;
features/storage/FEATURE_STORAGE/cfstore/source/cfstore_utest.h:using namespace utest::v1;
features/unsupported/dsp/dsp/dsp.h:using namespace dsp;
mbed.h:using namespace mbed;
mbed.h:using namespace std;
platform/FileSystemLike.h:    using FileSystemHandle::open;
rtos/Mail.h:using namespace rtos;
rtos/rtos.h:using namespace rtos;

It should probably also be added to the contributor guidelines not to put using statements in header files.

bmcdonnell-ionx · 2017-12-12T21:03:37Z

@0xc0170, what is the meaning of the "type: question" label?

0xc0170 · 2017-12-13T10:21:25Z

@0xc0170, what is the meaning of the "type: question" label?

I labelled it as question initially to answer why there is using statement in the main header file. Once you shared there are few more, this can be considered as a bug, and we will need to clean those.

pan- · 2017-12-14T10:02:44Z

@bmcdonnell-ionx using declarations are fine in the following occurrences listed:

drivers/Serial.h:    using SerialBase::read;
drivers/Serial.h:    using SerialBase::write;
drivers/UARTSerial.h:    using FileHandle::readable;
drivers/UARTSerial.h:    using FileHandle::writable;
platform/FileSystemLike.h:    using FileSystemHandle::open;

I believe events/mbed_events.h and rtos/rtos.h follows the same pattern exhibited in mbed.h. These header files are entry point that include all public definition of their modules and import them into the global namespace. That doesn't follows with C++ good practices but it may be easier for starters to get something done more easily.

However we may question using directives in files that do not expose the entirety of their modules:

features/filesystem/fat/FATFileSystem.h:using namespace mbed;
features/storage/FEATURE_STORAGE/cfstore/source/cfstore_utest.h:using namespace utest::v1;
features/unsupported/dsp/dsp/dsp.h:using namespace dsp;
rtos/Mail.h:using namespace rtos;

Similarly we may question inclusion of modules headers (mbed.h, mbed_events.h and rtos.h) in individual headers files. Professional users should be able to cherry pick the definitions they need without polluting the global namespace.

@0xc0170 Is there a clear reason to include the std namespace in mbed.h ? I have difficulties to identify the design principle behind that decision.

bmcdonnell-ionx · 2017-12-14T15:21:47Z

@pan-

using declarations are fine in the following occurrences listed:

drivers/Serial.h:    using SerialBase::read;
drivers/Serial.h:    using SerialBase::write;
drivers/UARTSerial.h:    using FileHandle::readable;
drivers/UARTSerial.h:    using FileHandle::writable;
platform/FileSystemLike.h:    using FileSystemHandle::open;

On closer inspection, it looks like these using statements are just specifying for these multiply-derived classes - within the scope of the classes - which of several inherited member functions by the same name to use as "their own". Assuming I've interpreted correctly, then I agree that these few should remain.

I believe events/mbed_events.h and rtos/rtos.h follows the same pattern exhibited in mbed.h. These header files are entry point that include all public definition of their modules and import them into the global namespace. That doesn't follows with C++ good practices but it may be easier for starters to get something done more easily.

(Emphasis mine.)

I don't think the modest gain for beginners here is worth the cost to others.

Besides, won't modifying the examples to have the using directives (or to fully specify identifiers) be sufficient guidance?

However we may question using directives in files that do not expose the entirety of their modules:

features/filesystem/fat/FATFileSystem.h:using namespace mbed;
features/storage/FEATURE_STORAGE/cfstore/source/cfstore_utest.h:using namespace utest::v1;
features/unsupported/dsp/dsp/dsp.h:using namespace dsp;
rtos/Mail.h:using namespace rtos;

Obviously I agree that these should go away. (Included for completeness.)

Similarly we may question inclusion of modules headers (mbed.h, mbed_events.h and rtos.h) in individual headers files.

What do you mean by "inclusion of modules headers...in individual header files"?

Professional users should be able to cherry pick the definitions they need without polluting the global namespace.

@0xc0170 Is there a clear reason to include the std namespace in mbed.h? I have difficulties to identify the design principle behind that decision.

pan- · 2017-12-14T17:06:59Z

What do you mean by "inclusion of modules headers...in individual header files"?

Here is an example with the ATParser header. It is not possible to import definitions present in that header without polluting the global namespace because it includes mbed.h rather than the individual headers required.

bmcdonnell-ionx · 2017-12-14T17:16:04Z

I'm still not clear on what you're saying. mbed.h is not a module.

It is not possible to import definitions present in that header

What would be an example of "importing a definition"?

...I'm not a fan of the single monolithic header file (mbed.h) for the whole library/OS either, but I'm less sure that that's a bad thing (compared to these using statements), and I figured it's a separate discussion.

bmcdonnell-ionx · 2017-12-16T19:45:31Z

@ciarmcom

ARM Internal Ref: IOTMORF-1817

What does this mean?

Also, what do the mirrored and tracking labels mean?

0xc0170 · 2017-12-18T09:06:21Z

That this has been added to our internal system. Nothing to be worry about

geky · 2017-12-18T17:31:58Z

The hard answer: Backwards compatibility.

There's no path for removing the using statements from mbed.h without breaking nearly every single line of code that has been written against mbed at this point.

That doesn't follows with C++ good practices but it may be easier for starters to get something done more easily.

This was the original idea I believe, and I think it was founded on a good idea. But it does cause problems.

What can we do?

I've been mulling over this thought for a while, and I think there is a path to fix the namespace. It may take a bit of work to implement, but I think the improved developer experience would be worth it:

Add a new file mbed-os.h
- Replaces mbed.h
- Includes every mbed API
- No using statement
Make sure all supported mbed APIs are in the mbed namespace
- Quite a few aren't currently, which is an annoying inconsistency
Remove nested namespaces unless they serve a specific purpose
- Flatten rtos + events namespace
- This can be done with the appropriate using statements to move the APIs into the mbed namespace directly
- The nested namespaces don't add much value over a functioning mbed namespace and adds more work on users to find the right names.
Keep mbed.h around for backwards compatibility

Afterwards, this:

#include "mbed.h"
blahblahblah

Turns into this:

#include "mbed-os.h"
using namespace mbed;
blahblahblah

Or something like this:

#include "mbed-os.h"
using mbed::NetworkInterface;
using mbed::TCPSocket;
blahblahblah

This would be a big change and would probably require someone dedicated to the task for a stretch of time, but it would remove concerns with name collisions. It would also present a more standard C++ API, and help establish the boundary between what is an mbed API and what is a target or user provided component.

This would be a big enough change that it may be worth considering for an mbed OS 6.0.0 release.

cc @sg-, @0xc0170, @pan- thoughts?

bmcdonnell-ionx · 2017-12-20T23:58:09Z

Example further motivation: I was just stumped for several minutes trying to forward declare class DigitalOut; in a header file because I forgot it was in the mbed namespace. If not for using namespace mbed; in mbed.h, I think the solution would've been more obvious.

pan- · 2018-01-08T15:19:41Z

@geky I'm not sure the discussion will lead somewhere

This was the original idea I believe, and I think it was founded on a good idea. But it does cause problems.

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.

The nested namespaces don't add much value over a functioning mbed namespace and adds more work on users to find the right names.

I would argue the contrary, nested namespace offers a clear separation between modules of the OS and prevent name clashes between them. It is easier to find an information if it's organized in cohesive collections of elements.

bmcdonnell-ionx · 2018-01-08T15:26:54Z

@pan-

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.

What do you think is the purpose of mbed-os.h, and why do you think it should not be used in production code?

pan- · 2018-01-08T16:39:54Z

why do you think it should not be used in production code?

Such facility increases builds time for very little benefits. It may be OK for some projects but not all of them. As an example consider the numbers given in the blog post covering the release of mbed os 5.7:

The total number of binaries built is 17,869,800 since the Mbed OS 5.6.0 release.

If using mbed-os.h instead of the specific headers increases build time by only 1 second then 206 days of CI CPU time is lost in 90 days. That's not negligible.

More generally, if we have a global headers use cases well as dos and don'ts should be documented. User can make their choices based on these information.

bmcdonnell-ionx · 2018-01-08T16:55:27Z

Oh, good points.

I actually alluded to this earlier, when I said,

I'm not a fan of the single monolithic header file (mbed.h) for the whole library/OS either, but ... I figured it's a separate discussion.

Since you brought it up again, I'd just as soon see mbed.h go away, and have users #include the modules they need. It can be a little more work when initially writing the code to find things, but it would increase code clarity, improve modularity, and decrease build times.

Of course, documentation would need to be updated accordingly. A list of modules and corresponding header names to #include would be nice. I don't know the roadmap, but maybe it would fit in with the mbed OS 6.0 release.

ciarmcom · 2018-06-19T11:52:21Z

ARM Internal Ref: MBOTRIAGE-791

bmcdonnell-ionx · 2018-08-03T15:41:44Z

@deepikabhavnani, please re-open. Several using namespace statements have still not been removed from header files.

me@pc MINGW64 /c/dev/mbed-os (master)
$ find . -name "*.h" -exec grep -R ^" *using .*namespace" {} +
./events/mbed_events.h:using namespace events;
./features/storage/FEATURE_STORAGE/cfstore/source/cfstore_utest.h:using namespace utest::v1;
./features/unsupported/dsp/dsp/dsp.h:using namespace dsp;
./mbed.h:using namespace mbed;
./mbed.h:using namespace std;
./rtos/Mail.h:using namespace rtos;
./rtos/rtos.h:using namespace rtos;

me@pc MINGW64 /c/dev/mbed-os (master)
$ git log -n1 --oneline
546dafbad (HEAD -> master, origin/master, origin/HEAD) Merge pull request #7687 from paul-szczepanek-arm/fix-default-privacy

deepikabhavnani · 2018-08-03T15:54:10Z

@bmcdonnell-ionx - Sorry missed few, but from previous conversation, below will not be fixed till major version release for backward compatibility.

./mbed.h:using namespace mbed;
./mbed.h:using namespace std;

bmcdonnell-ionx · 2018-08-03T15:56:55Z

@deepikabhavnani, should one of us open a new issue for that after you fix the others? Or should this issue stay open for those?

deepikabhavnani · 2018-08-03T15:58:12Z

Let this issue be open. I don;t think "./features/unsupported/dsp/dsp/dsp.h:using namespace dsp;" this will be fixed as it is unsupported.

deepikabhavnani · 2018-08-03T15:58:56Z

Pending ones listed below:

~~./events/mbed_events.h:using namespace events;~~

./features/storage/FEATURE_STORAGE/cfstore/source/cfstore_utest.h:using namespace utest::v1;
./rtos/Mail.h:using namespace rtos;

~~./rtos/rtos.h:using namespace rtos;~~

bmcdonnell-ionx · 2018-08-03T16:00:11Z

What I mean is, after those 4 are fixed, should this issue still remain open due to the using statements in mbed.h, or should a new one be opened for that?

deepikabhavnani · 2018-08-03T16:12:57Z

ohh.. Yeah it would be good to have new issue (enhancement) for mbed.h change. Thanks :-)
Edit:
Was looking into previous comments and saw behavior of events and rtos is same as mbed.h. Hence they should be included in new issue as well.

./rtos/rtos.h:using namespace rtos;
./events/mbed_events.h:using namespace events;
./mbed.h:using namespace mbed;
./mbed.h:using namespace std;

bmcdonnell-ionx · 2018-08-06T21:22:11Z

So these two remain on this issue.

(1) ./features/storage/FEATURE_STORAGE/cfstore/source/cfstore_utest.h:using namespace utest::v1;
(2) ./rtos/Mail.h:using namespace rtos;

In an effort to wrap this up...

The Mail example builds fine when I remove that line from (2). Since rtos.h also has using namespace rtos, I don't think this will break anything.

Re (1), how do I build and run the cfstore tests to test this? I tried mbedgt --test-spec test_spec.json with this test_spec.json (rename to use), but I get errors.

$ mbedgt --test-spec test_spec.json
mbedgt: greentea test automation tool ver. 1.4.0
mbedgt: test specification file 'test_spec.json' (specified with --test-spec option)
mbedgt: using 'test_spec.json' from current directory!
mbedgt: detecting connected mbed-enabled devices...
mbedgt: detected 1 device
mbedgt: processing target 'LPC4088' toolchain 'GCC_ARM' compatible platforms... (note: switch set to --parallel 1)
mbedgt: running 1 test for platform 'LPC4088' and toolchain 'GCC_ARM'
mbedgt: mbed-host-test-runner: started
Exception in thread Thread-3:
Traceback (most recent call last):
  File "C:\Users\me\AppData\Local\Programs\Python\Python37\lib\threading.py", line 917, in _bootstrap_inner
    self.run()
  File "C:\Users\me\AppData\Local\Programs\Python\Python37\lib\threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
  File "C:\Users\me\AppData\Local\Programs\Python\Python37\lib\site-packages\mbed_greentea\mbed_greentea_cli.py", line 489, in run_test_thread
    verbose=verbose)
  File "C:\Users\me\AppData\Local\Programs\Python\Python37\lib\site-packages\mbed_greentea\mbed_test_api.py", line 318, in run_host_test
    returncode, htrun_output = run_htrun(cmd, verbose)
  File "C:\Users\me\AppData\Local\Programs\Python\Python37\lib\site-packages\mbed_greentea\mbed_test_api.py", line 138, in run_htrun
    test_error = htrun_failure_line.search(line)
TypeError: cannot use a string pattern on a bytes-like object

mbedgt: could not generate test report
mbedgt: completed in 1.80 sec
mbedgt: exited with code -1000

0xc0170 · 2018-08-07T14:45:04Z

Thanks @bmcdonnell-ionx for the summary , we should address both.

adbridge · 2018-10-04T12:22:31Z

Internal Jira reference: https://jira.arm.com/browse/IOTCORE-525

deepikabhavnani · 2018-11-27T16:03:19Z

@bmcdonnell-ionx - #7760 should resolve this issue.

All using statements cannot be removed from all header files for backward compatibility, but you can add 'MBED_NO_GLOBAL_USING_DIRECTIVEdefine to build withoutusing namespace`. Also using specific header files (not mbed.h / rtos.h) should help.

Closing this. Please reopen new issue, if more namespace issues are encountered.

Thanks for your patience and support in resolving this.

bmcdonnell-ionx changed the title ~~header files should not have "using" statements (namespaces)~~ header files should not have using statements (namespaces) Dec 8, 2017

0xc0170 added the type: question label Dec 11, 2017

0xc0170 added type: bug and removed type: question labels Dec 13, 2017

sg- added component: drivers tracking labels Dec 16, 2017

ciarmcom added the mirrored label Dec 16, 2017

bmcdonnell-ionx mentioned this issue Jan 30, 2018

Complete mbed_retarget FileHandle rework #5571

Merged

sg- added type: enhancement and removed type: bug labels Feb 13, 2018

This was referenced Mar 22, 2018

SerialWireOutput class outside of mbed namespace #6421

Closed

Added SerialWireOutput to mbed namespace #6427

Merged

bmcdonnell-ionx mentioned this issue Apr 19, 2018

BlockDevice classes should be moved to mbed namespace #6684

Closed

0xc0170 mentioned this issue Jun 19, 2018

"using namespace" is inside of mbed.h #7248

Closed

0xc0170 removed mirrored tracking labels Jun 19, 2018

ARMmbed deleted a comment from ciarmcom Jun 19, 2018

ciarmcom added the mirrored label Jun 19, 2018

deepikabhavnani mentioned this issue Jul 31, 2018

Remove inclusion of mbed.h and mbed namespace from filesystem code #7660

Merged

deepikabhavnani closed this as completed Aug 3, 2018

ciarmcom added the closed_in_jira label Aug 3, 2018

deepikabhavnani reopened this Aug 3, 2018

bmcdonnell-ionx mentioned this issue Aug 21, 2018

Give an option to remove 'using namespace' #7760

Merged

adbridge added Jira status: OPEN and removed closed_in_jira labels Oct 3, 2018

ciarmcom added Jira status: IN PROGRESS and removed Jira status: OPEN labels Oct 23, 2018

deepikabhavnani closed this as completed Nov 27, 2018

ciarmcom added Jira status: CLOSED and removed Jira status: IN PROGRESS labels Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bmcdonnell-ionx commented Dec 8, 2017

0xc0170 commented Dec 11, 2017

bmcdonnell-ionx commented Dec 11, 2017 •

edited

Loading

bmcdonnell-ionx commented Dec 12, 2017

0xc0170 commented Dec 13, 2017

pan- commented Dec 14, 2017

bmcdonnell-ionx commented Dec 14, 2017

pan- commented Dec 14, 2017

bmcdonnell-ionx commented Dec 14, 2017

bmcdonnell-ionx commented Dec 16, 2017

0xc0170 commented Dec 18, 2017

geky commented Dec 18, 2017

bmcdonnell-ionx commented Dec 20, 2017

pan- commented Jan 8, 2018

bmcdonnell-ionx commented Jan 8, 2018 •

edited

Loading

pan- commented Jan 8, 2018

bmcdonnell-ionx commented Jan 8, 2018 •

edited

Loading

ciarmcom commented Jun 19, 2018

bmcdonnell-ionx commented Aug 3, 2018 •

edited

Loading

deepikabhavnani commented Aug 3, 2018

bmcdonnell-ionx commented Aug 3, 2018

deepikabhavnani commented Aug 3, 2018

deepikabhavnani commented Aug 3, 2018 •

edited

Loading

bmcdonnell-ionx commented Aug 3, 2018

deepikabhavnani commented Aug 3, 2018 •

edited

Loading

bmcdonnell-ionx commented Aug 6, 2018

0xc0170 commented Aug 7, 2018

adbridge commented Oct 4, 2018

deepikabhavnani commented Nov 27, 2018

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

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

Comments

bmcdonnell-ionx commented Dec 8, 2017

Description

0xc0170 commented Dec 11, 2017

bmcdonnell-ionx commented Dec 11, 2017 • edited Loading

bmcdonnell-ionx commented Dec 12, 2017

0xc0170 commented Dec 13, 2017

pan- commented Dec 14, 2017

bmcdonnell-ionx commented Dec 14, 2017

pan- commented Dec 14, 2017

bmcdonnell-ionx commented Dec 14, 2017

bmcdonnell-ionx commented Dec 16, 2017

0xc0170 commented Dec 18, 2017

geky commented Dec 18, 2017

bmcdonnell-ionx commented Dec 20, 2017

pan- commented Jan 8, 2018

bmcdonnell-ionx commented Jan 8, 2018 • edited Loading

pan- commented Jan 8, 2018

bmcdonnell-ionx commented Jan 8, 2018 • edited Loading

ciarmcom commented Jun 19, 2018

bmcdonnell-ionx commented Aug 3, 2018 • edited Loading

deepikabhavnani commented Aug 3, 2018

bmcdonnell-ionx commented Aug 3, 2018

deepikabhavnani commented Aug 3, 2018

deepikabhavnani commented Aug 3, 2018 • edited Loading

bmcdonnell-ionx commented Aug 3, 2018

deepikabhavnani commented Aug 3, 2018 • edited Loading

bmcdonnell-ionx commented Aug 6, 2018

0xc0170 commented Aug 7, 2018

adbridge commented Oct 4, 2018

deepikabhavnani commented Nov 27, 2018

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

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

bmcdonnell-ionx commented Dec 11, 2017 •

edited

Loading

bmcdonnell-ionx commented Jan 8, 2018 •

edited

Loading

bmcdonnell-ionx commented Jan 8, 2018 •

edited

Loading

bmcdonnell-ionx commented Aug 3, 2018 •

edited

Loading

deepikabhavnani commented Aug 3, 2018 •

edited

Loading

deepikabhavnani commented Aug 3, 2018 •

edited

Loading