Fix for issue #419

[lyytinen]

This pull request fixes issue #419.

The implementation is not optimal as it removes the implicit conversion operator from MSVC builds. Conversions to boost::optional<uint16_t> should therefore done explicitly from here on.

[glynos]

I can test this using VS2012, so I'll take a look once the travis CI build shows success.

[glynos]

The travis CI build is taking some time, but I can confirm that the linearize tests are now passing on my machine with VS2012.

FWIW, I think the original code has a bad smell to it, as it relies on an implicit conversion depending on the return type. This makes it incredibly brittle and I'm not so surprised that it eventually broke.

[glynos]

@deanberris Once the travis build is complete, I can confirm that it works on MSVC 2012, and would be happy to merge the changes, if you're OK with them yourself.

[deanberris]

I actually would like to have the changes hidden to only MSVC. The thing I'm worried about is the fact that others may already be using this relying on the concept. Is that possible to do?

FWIW @glynos this pattern is actually very useful -- intent is clear in an expressive manner. The reality here is that the bug is in the compiler, instead of the actual code.

I agree it's not common, but it's incredibly expressive. This is as close as return-type overload resolution C++ is going to get.

[lyytinen]

I don't know how to limit this to MSVC alone and providing different APIs for different platforms actually sounds equally bad to me. Perhaps there's a way but someone else will have to jump in here.

In general, I feel that expressiveness often comes at the cost of readability and maintainability. Considering that boost::optional has the following assignment operators:

optional& operator = ( T const& v ) ;
optional& operator = ( optional const& rhs ) ;

And that the port_wrapper implements the following implicit conversions:

operator T() const
operator optional() const

I'm not that versed in C++ that I can easily say which conversion should take precedence when making the assignment. So IMO the intent is not that clear here.

Also note that after removing the implicit conversion to optional the following will still compile in MSVC but the semantics will change:

boost::optionalboost::uint16_t port_ = port(request);

I would therefore favor explicit conversions over implicit ones.

[deanberris]

Looking at the assignment operator is actually not the right thing to do here -- what the code is doing is a copy initialization, and it picks among overloads of the constructors that fit. In those cases the most suitable conversion is the one that doesn't require an extra step (because the uint16_t constructor is marked explicit). Other compilers behave correctly here favoring the type's copy constructor because the type of the expression on the right of the initializer is convertible through a conversion operator.

MSVC seems to still consider conversion operator for the uint16_t case because it thinks that it can do a conversion even for the explicit constructor (which is actually a bug). This is why the ambiguity arises in MSVC only.

You can actually isolate the fix you're doing just to MSVC by following the #if (_MSC_VER >= 1600) ... #else ... #endif pattern already in the code you're changing.

Note that you can let Travis check that you haven't broken anything because it does so for GCC and Clang on Linux. I'm not sure it's possible to get Travis to run it on Windows too (if that's even an option). :)

Cheers

[deanberris]

This one in particular you may want to wrap in something like this:

boost::optional<boost::uint16_t> port_ =
#if (_MSC_VER >= 1600)
  port(port).as_optional();
#else
  port(port);
#endif

[deanberris]

LGTM

Thanks @lyytinen for doing this!