search

ptal / expected

What did you expect?
113 stars 18 forks source link

Fixed bug involving static_addressof #74

Closed Fraser999 closed 9 years ago

Fraser999 commented 9 years ago

The test I added exposes a bug in expected.hpp where the calls to static_addressof need to be prefixed with detail::.

However if that's applied, with MSVC 2013, the calls to has_overloaded_addressof::value generate the following error:

.../boost/expected/detail/static_addressof.hpp(34): error C2057: expected constant expression

due to the lack of constexpr support.

I couldn't see a reason to not use std::addressof in place of detail::static_addressof, but I might well be missing something. If so, feel free to reject the patch and I can just file an issue separately if you want.

Finally, I deleted the const overloads of errorptr() since they're unused. errorptr() seems to be useful only in the context of non-const functions like the move c'tor or swap.

viboes commented 9 years ago

Hi,

I don't want to replace the static_addressof. We could have a flag that makes it use std::addressof if the user prefers it. Do you mind to prepare this patch?

Could you provide two PR? one for the test and one for the patch?

Thanks, Vicente

Fraser999 commented 9 years ago

Sure thing.

I guess we could call the flag BOOST_EXPECTED_USE_STD_ADDRESSOF and define it similarly to BOOST_EXPECTED_NO_CXX11_MOVE_ACCESSORS so it's defaulted for MSVC <= 2013?

Fraser999 commented 9 years ago

Sorry - I just reread my comment and I didn't mean completely as per BOOST_EXPECTED_NO_CXX11_MOVE_ACCESSORS, I just meant to define it for _MSC_VER < 1900. Do you know if there's a minimum version of Clang or GCC for which it should be defined too? I only have up-to-date versions of these handy.

Fraser999 commented 9 years ago

OK - I've created https://github.com/ptal/expected/pull/75 and https://github.com/ptal/expected/pull/76, so I'll close this one.

I've left in the const overloads of errorptr() since I guess they're not doing any harm.

Fraser999 commented 9 years ago

Yeah - I didn't see your comment before I closed this. I've already fixed that in my new PR - obviously I took the more tedious path and defined a default c'tor rather than removing the const :-)

The test passes on OS X (Clang 3.5) and MSVC 2013 after both PRs are merged.

viboes commented 9 years ago

For your information, the need of this static_address_of is because std::address_of is not constexpr.