Mbed-os: header files should not have `using` statements (namespaces)

[bmcdonnell-ionx]

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.

[0xc0170]

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]

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]

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

[0xc0170]

@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-]

@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]

@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-]

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]

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]

@ciarmcom

ARM Internal Ref: IOTMORF-1817

What does this mean?

Also, what do the mirrored and tracking labels mean?

[0xc0170]

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

[geky]

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:

1. Add a new file mbed-os.h
   
   
   
   • Replaces mbed.h
   
   
   • Includes every mbed API
   
   
   • No using statement
   
   
2. Make sure all supported mbed APIs are in the mbed namespace
   
   
   
   • Quite a few aren't currently, which is an annoying inconsistency
   
   
3. 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.
   
   
4. 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]

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-]

@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]

@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-]

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]

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]

ARM Internal Ref: MBOTRIAGE-791

[bmcdonnell-ionx]

@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]

@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]

@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]

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]

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]

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]

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]

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]

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

[adbridge]

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

[deepikabhavnani]

@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.