OpenZWave / open-zwave

a C++ library to control Z-Wave Networks via a USB Z-Wave Controller.
http://www.openzwave.net/
GNU Lesser General Public License v3.0
1.05k stars 916 forks source link

I think Defs.h should not import std ("using namespace std") #2169

Open petergebruers opened 4 years ago

petergebruers commented 4 years ago

This might be a minor issue, but maybe 1.7 can fix this.

I compile this code

#include <Manager.h>

using namespace OpenZWave;

int main()
{
    cout << "test\n";
}

It compiles... Usually people are happy when things compile but in this case I am not ๐Ÿ˜‰ - "cout" is definitely not in the OpenZWave namespace so it looks line namespace std was silently imported... It should fail with "error: use of undeclared identifier 'cout';"

g++ -c -g ozw_namespace.cpp -std=c++11 -I /usr/local/include/openzwave -E > preproc.txt

Then in preproc.txt:

# 175 "/usr/local/include/openzwave/Defs.h"
namespace std
{
}
namespace OpenZWave
{

 using namespace std;
 namespace Internal
 {
  namespace CC
  {

  }
 }
}

So the culprit is in Defs.h

This makes OpenZWave::cout an alias for std::cout and everything else in std and by doing 'using namespace OpenZWave" (which may not be good practice but likely gets used a lot) suddenly the unqualified "cout" becomes OpenZWave::cout and then std::cout

I do not have a lot of experience with this so I googled it and I would say it is best practice to avoid importing namespaces in header files

https://stackoverflow.com/questions/5849457/using-namespace-in-c-headers

Of course, if you remove the "using namespace std;" from Defs.h - OZW no longer compiles because many cpp files do not import namespace std explicitly, fo example

In file included from /Users/peter/ozw/open-zwave/cpp/src/command_classes/AssociationCommandConfiguration.cpp:28:
/Users/peter/ozw/open-zwave/cpp/src/command_classes/CommandClasses.h:56:13: error: unknown type name 'string'; did you mean 'std::string'?

Unfortunately this means also all header files using "string" and "map" now need "std::string" and "std::map" and so on. It is a lot of work.

This means a PR for this would create a lot of code noise, the (only) benefits would be "remove ambiguity" and "possible compilation errors due to name resolution" if I am correct. So far I have only seen a few complaints that OZW causes issues and I am not sure if they are caused by this. IIRC it is about redefinition of time function on the windows platform. Removing theusing namespace would cause no functional change to OZW.

If you flag this as a non-issue, I can understand that. But I went to the trouble of diagnosing this anyway and so I thought: "I might as well report this and voice my opinion" ๐Ÿ˜„

Fishwaldo commented 4 years ago

Yeah - This is Legacy, but agree it should be fixed.

I'll aim todo it towards the end of the 1.7 Dev Cycle.