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 access violation that happens when Mock with destructor has more methods. #144

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

ericlemes
Copy link
Collaborator

If the offsets are all initialised to 0, ocasionally the destructors get the wrong offset.

… mocked interface.

If the offsets are all initialised to 0, ocasionally the destructors get the wrong offset.
@coveralls
Copy link

Coverage Status

Coverage decreased (-100.0%) to 0.0% when pulling 5fb5a87 on ericlemes:master into 33022fe on eranpeer:master.

@ericlemes
Copy link
Collaborator Author

Bad coverage, but seems to be a problem in the tool.

@ericlemes
Copy link
Collaborator Author

I noticed also that the same tests will fail if they are compiled with /Zi instead of /ZI (Debug Information Format under C/C++ options).

I think either needs some more investigation or a documentation changed or a static_assert to avoid compiling with wrong options.

This has been tested on VC++ 15.6.x

@eranpeer eranpeer merged commit 5dd30de into eranpeer:master Jul 25, 2018
@eranpeer
Copy link
Owner

This was a very important fix. I was looking for it for a long time. Thanks, that was great help.

@ericlemes
Copy link
Collaborator Author

@eranpeer good to know. It was definitely not a simple bug. The other comment about the /Zi on Windows also took me some serious weeks to spot. So, it would be good to know if you are interested in contributions because I have some heavy users and loads of interesting use cases, but since you were a bit unresponsive, we started using a fork.

@eranpeer
Copy link
Owner

eranpeer commented Jul 25, 2018 via email

@ericlemes
Copy link
Collaborator Author

That would be really good. I just need to make sure we are aligned in terms of direction, to make sure I keep the right philosophy. But that would be good.

I personally don't like the idea of forking projects. I think making contributions and working together usually gives better results and stronger communities.

@eranpeer
Copy link
Owner

Great, I will add you as admin, you will be able to approve pull requests and push updates without me being involved.

@eranpeer
Copy link
Owner

eranpeer commented Jul 25, 2018

Whenever you want to make functional changes it will be best if we have a discussion first. We want to keep the API and the code as simple and clean as possible. Obviously, everything must be fully tested. Beside that there is no big philosophy.

@ericlemes
Copy link
Collaborator Author

That's cool, thanks!

One thing I was thinking to add in terms of feature is something similar for AlwaysReturn and Return that handles classes without copy constructors. I've noticed you can workaround with AlwaysDo, but it is kind a lot of work.

@eranpeer
Copy link
Owner

I didn't find a workaround for classes without a copy constructor. If you know how to do it, it can be a great enhancement. Try building a POC first, I may be able to help it it is a lot of work.

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.

None yet

3 participants