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

Fix operator== overload ambiguity #1067

Closed
wants to merge 1 commit into from

Conversation

th0br0
Copy link

@th0br0 th0br0 commented Apr 24, 2018

Currently, json does not work well together with GoogleMock.
(e.g. MOCK_METHOD0(sample, nlohmann::json(void));)

This is because of an ambiguous overload which this PR fixes by disabling one of the two.
As std::is_null_pointer is only available from C++14, this uses the possible implementation from http://en.cppreference.com/w/cpp/types/is_null_pointer

Example error:

In file included from external/com_google_googletest/googlemock/include/gmock/gmock.h:58:
external/com_google_googletest/googlemock/include/gmock/gmock-actions.h:388:18: error: use of overloaded operator '==' is ambiguous (with operand types 'const internal::linked_ptr<ActionInterface<const basic_json<std::map, std::vector, basic_string<char, char_traits<char>, allocator<char> >, bool, long, unsigned long, double, std::allocator, adl_serializer> ()> >' and 'nullptr_t')
    return impl_ == nullptr && fun_ == nullptr;
           ~~~~~ ^  ~~~~~~~
SNIP

external/com_google_googletest/googletest/include/gtest/internal/gtest-linked_ptr.h:186:8: note: candidate function
  bool operator==(T* p) const { return value_ == p; }
       ^
json.h:15149:17: note: candidate function [with ScalarType = nullptr_t, $1 = 0]
    friend bool operator==(const_reference lhs, const ScalarType rhs) noexcept
                ^
json.h:15078:17: note: candidate function
    friend bool operator==(const_reference lhs, const_reference rhs) noexcept

@coveralls
Copy link

coveralls commented Apr 25, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 435fc32 on th0br0:fix/overload into b5d1755 on nlohmann:develop.

@nlohmann
Copy link
Owner

Is there a way to add a test for the change?

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test case for the change?

@th0br0
Copy link
Author

th0br0 commented Apr 27, 2018

I've been thinking about how best to approach this; a basic equality comparison with nullptr doesn't do the trick. I'll try to dive into gmock to figure out a repro.

@nlohmann
Copy link
Owner

@th0br0 Do you have an idea how to proceed with this PR?

@nlohmann
Copy link
Owner

Any news on this?

@nlohmann
Copy link
Owner

@th0br0 Any idea how to proceed here?

@nlohmann
Copy link
Owner

@th0br0 As for a test case: couldn't you just see which call Google Mock made when you detected the error and use it as regression test for your fix?

@theodelrieu
Copy link
Contributor

I just stumbled on this issue, and it looks very similar to what I've described in #960.

Here, the compiler considers converting an internal GMock type to json, because json is inside its template arguments:

const internal::linked_ptr<ActionInterface<const basic_json<...>>>

Since ADL looks at the template arguments too, so you have the same issue. I've explained in detail why it happens in #960.

@stale
Copy link

stale bot commented Aug 22, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 22, 2018
@stale stale bot closed this Aug 29, 2018
@theodelrieu theodelrieu mentioned this pull request Sep 6, 2018
4 tasks
@nlohmann
Copy link
Owner

nlohmann commented Sep 9, 2018

@th0br0 This issue could be fixed after merging #1228. Could you try using the latest develop version whether the issue is fixed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants