zaphoyd / websocketpp

C++ websocket client/server library
http://www.zaphoyd.com/websocketpp
Other
6.97k stars 1.97k forks source link

_WEBSOCKETPP_CPP11_SYSTEM_ERROR_ shouldn't be defined by default when using Boost.Asio #651

Open vadz opened 7 years ago

vadz commented 7 years ago

common/system_error.hpp defines _WEBSOCKETPP_CPP11_SYSTEM_ERROR_ when using a C++11 compiler (or MSVS 2010+) by default, ensuring that the detailed information about error codes is lost when using Boost.Asio, as boost::system::error_code used by it can't be translated to std::error_code and so a catch all and rather useless error::pass_through is used for all of them. IMO this is a big drawback which totally negates any benefits from using the standard class.

Of course, it's simple enough to work around this by predefining _WEBSOCKETPP_NO_CPP11_SYSTEM_ERROR_, but I think this shouldn't be done by default but only if ASIO_STANDALONE is defined, in which case nothing would be lost.

zaphoyd commented 7 years ago

I agree that this would be a good change. I'm trying to think of a clean way to actually do it though.

At the time system_error.hpp is included the only signal the library has about what transport is being used is ASIO_STANDALONE. This tells us that we should be using std::error_code, but that was the default anyways. The lack of ASIO_STANDALONE isn't sufficient to confirm that we are using Boost Asio.

The best way to do this check is in common/asio.hpp, but that generally gets included after common/system_error.hpp.

Edit: to be more clear: There are situations where neither Boost Asio, nor Standalone Asio are present and we need to use STL system_error. For this reason boost system_error can't be a blanket default.

vadz commented 7 years ago

Is it really likely for the library to be used without either Boost ot standalone Asio?

If this use case is really important, another define such BOOST_ASIO is clearly needed. Or maybe a 3-valued USE_ASIO define with values indicating whether the standard, Boost or no version is used. But we clearly must distinguish the case of Boost.Asio from the other ones somehow.

zaphoyd commented 7 years ago

The library absolutely is used without Asio at all by many people (this is the whole reason it has a pluggable transport system =) ). At minimum, most of the unit tests of the core library are built without Asio.

A user level define that would signal "Intent to use Boost Asio" in the same way that ASIO_STANDALONE signals intent to use standalone Asio would solve the problem. It isn't the cleanest (still requires the end user to do something) but I think its much better than trying to explain... "if you are using Boost Asio you can get better error messages by defining _WEBSOCKETPP_NO_CPP11_SYSTEM_ERROR_."