Skip to content

Added comparison operator to FunctionPointerBase. #52

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

Closed
wants to merge 1 commit into from
Closed

Added comparison operator to FunctionPointerBase. #52

wants to merge 1 commit into from

Conversation

marcuschangarm
Copy link
Contributor

@bremoran
Copy link
Collaborator

+1

@@ -29,6 +29,11 @@ class FunctionPointerBase {
return (_membercaller != NULL) && (_object != NULL);
}

bool operator==(const FunctionPointerBase& other)
{
return (_membercaller == other._membercaller) && (_object == other._object);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really work properly? membercaller ends up taking the address of a static function, which is the same for all instances of FunctionPointerX<T> for a given T. So, for a given instance inst of type T, wouldn't this return true for 2 function pointers that reffer to different methods of T? Sorry if I'm missing something.

@autopulated
Copy link
Contributor

Does this really work properly?

I don't think it does. There's also the issue of bound arguments (since FunctionPointerBind derives from this): should they be tested for equality? If so, by POD-value, or by recursively calling operator== (presumably the latter)?

@marcuschangarm
Copy link
Contributor Author

@bogdanm I see your point. It "works" in my app because testing for object is sufficient in my case.

I'm not sure how to make it work properly though.

@bogdanm
Copy link
Contributor

bogdanm commented Jul 21, 2015

As @autopulated pointed out, making it work properly is not trivial, especially the FunctionPointerBind part. I guess we could refactor the code a bit and make FunctionPointerBind include a FunctionPointerBase as opposed to deriving from it, which would simplify the implementation of operator == a bit easier (see also #28)

@autopulated
Copy link
Contributor

It's possible to declare a non-member operator== that takes two arguments, if that helps...

@marcuschangarm
Copy link
Contributor Author

Regarding FunctionPointerBind, testing the arguments for equality and then the derived FunctionPointerBase should be sufficient?

@autopulated
Copy link
Contributor

Yes, assuming the base implementation distinguishes different member functions.

@marcuschangarm
Copy link
Contributor Author

I tried to compare the content of uintptr_t _member[4] in FunctionPointerBase but that didn't seem to work, which I don't understand why it didn't.

@autopulated
Copy link
Contributor

actually... not sure why that wouldn't work. Should be different for different member functions, is it the same?

@marcuschangarm
Copy link
Contributor Author

It's different for members that should be the same.

@marcuschangarm
Copy link
Contributor Author

as in this didn't work:

bool operator==(const FunctionPointerBase& other)
{
    return (_member[0] == other._member[0]) &&
              (_member[1] == other._member[1]) &&
              (_member[2] == other._member[2]) &&
              (_member[3] == other._member[3]) &&
              (_object == other._object);
}

@autopulated
Copy link
Contributor

Ah – it might include uninitialised memory if the size of the member function pointer is different? member function pointers can even be different sizes depending on the sort of function they point to. See http://www.codeproject.com/Articles/7150/Member-Function-Pointers-and-the-Fastest-Possible

@autopulated
Copy link
Contributor

although apparently not in gcc, hmm :/

@bremoran
Copy link
Collaborator

The only way to handle this safely is to provide a comparison operator static function in FunctionPointer.

@bogdanm
Copy link
Contributor

bogdanm commented Jul 21, 2015

@marcuschangarm, maybe that's an initialization problem? I don't think _member is ever initialized to 0 anywhere?

@marcuschangarm
Copy link
Contributor Author

I added that, but it didn't help.

I'll do some more debugging when I have the cycles, at the moment just checking the object works in my app.

But it sounds like comparing _member and _object should do the trick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants