Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Equality check does not work on Windows #111

Merged
merged 3 commits into from Apr 10, 2019

Conversation

2 participants
@jmjatlanta
Copy link

commented Mar 4, 2019

Part of /bitshares/bitshares-core/issues/1593

Compiling on Windows causes an error. It seems the compiler cannot determine which equality operator to use given the arguments. As a "fix", the preprocessor removes this test line from Windows builds.

"E:\bitshares-core-hardfork\install.vcxproj" (default target) (1) ->
"E:\bitshares-core-hardfork\ALL_BUILD.vcxproj" (default target) (3) ->
"E:\bitshares-core-hardfork\libraries\fc\tests\all_tests.vcxproj" (default target) (4) ->
(ClCompile target) ->
E:\bitshares-core-hardfork\libraries\fc\tests\variant_test.cpp(73): error C2666: 'fc::test::operator ==': 6 overloads have similar conversions [E:\bitshares-core-hardfork\libraries\fc\tests\all_tests.vcxproj]

I do not like this fix, as it is not testing a feature that could be used elsewhere (which I assume would also fail to compile). It looks as if the way equality is done here is broken (on Windows), and needs to be fixed. Input from others would be appreciated.

@pmconrad

This comment has been minimized.

Copy link

commented Mar 5, 2019

Hm, I'd have thought that static_variant in a map or set uses comparison operators. Maybe only < not ==? But the declaration of friend operator< in static_variant is quite similar to friend operator==...

I think what happens here is that init_value is auto-converted to a static_variant using the single-arg constructor. Maybe VS considers the single-arg constructors of all known static_variants a valid choice and gets confused.

Have you tried changing the operator== declaration to

friend bool operator == ( const static_variant<Types...>& a, const static_variant<Types...>& b )

?

@jmjatlanta

This comment has been minimized.

Copy link
Author

commented Mar 5, 2019

Error in VisualStudio:

more than one operator "==" matches these operands: e:\bitshares-fc\tests\variant_test.cpp(73) 
function "fc::operator==(const fc::unsigned_int &i, const uint64_t &v)"
function "fc::operator==(const fc::static_variant<int64_t, std::string, fc::variant> &a, const fc::static_variant<int64_t, std::string, fc::variant> &b)"
operand types are: fc::static_variant<int64_t, std::string, fc::variant> == sv::tag_type

So it seems to me it cannot decide between turning variant_with_tag_type into an fc::unsigned_int and turning init_value into a static_variant.

Adding the <Types...> to the operator == didn't seem to help. Changing BOOST_CHECK(variant_with_tagtype == init_value); to BOOST_CHECK(variant_with_tagtype.get<sv::tag_type>() == init_value); works. But I still don't think that tests what we want to test.

jmjatlanta added some commits Mar 5, 2019

@pmconrad

This comment has been minimized.

Copy link

commented Mar 6, 2019

Hm, init_value is a int64_t...

BOOST_CHECK(variant_with_tagtype.get<sv::tag_type>() == init_value); works.

That just compares two integers. I agree this is not what we want to test here because we already test it in the line above that.
What other compilers are probably doing here is
BOOST_CHECK(variant_with_tagtype.get<sv::tag_type>() == fc::static_variant< sv::tag_type, std::string, fc::variant >(init_value))

@pmconrad

This comment has been minimized.

Copy link

commented Apr 10, 2019

@jmjatlanta is this still required or was it resolved in one of your other PRs? Feel free to merge.

@jmjatlanta jmjatlanta merged commit 5f8f799 into master Apr 10, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jmjatlanta

This comment has been minimized.

Copy link
Author

commented Apr 10, 2019

Unfortunately, it is still required. The good news is that if this equality check is used, the build will break. That is also the bad news.

@jmjatlanta jmjatlanta deleted the jmj_1593_variant branch Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.