search

eranpeer / FakeIt

C++ mocking made easy. A simple yet very expressive, headers only library for c++ mocking.
MIT License
1.22k stars 170 forks source link

Extended lifetime of actions to match those of mocks. #301

Closed FranckRJ closed 1 year ago

FranckRJ commented 1 year ago

Should fix #254.

coveralls commented 1 year ago

Coverage Status

Coverage increased (+4.0e-05%) to 99.925% when pulling 53a43f979da22cf68954e6defb5ccb9edca914c3 on extend-action-lifetime into 26e86dec53a597508de8e3dc35f5d431875b5de9 on dev-2.3.1.

FranckRJ commented 1 year ago

Ok so the extension of the lifetime of actions was done to fix a very specific bug (throwing string literals), but then it seems that the buggy code isn't even standard compliant (you cannot copy arrays by doing a2 = a1), so it shouldn't compile but only MSVC is standard-compliant there.

Not sure if I still want to merge this change. I guess I won't merge in for the 2.3.1 but will look at it later, because maybe it can fix other things.

malcolmdavey commented 1 year ago

This looks like the splitting out of the change from my PR, so it is useful for more than the case you referred to.

The important case is not for arrays but when we mock a function by giving it a copy to store for a return value where the signature is returned by a ref or const ref, and hence we do need to extend the life time.

I would say merge it is fine to merge now.

FranckRJ commented 1 year ago

The important case is not for arrays but when we mock a function by giving it a copy to store for a return value where the signature is returned by a ref or const ref, and hence we do need to extend the life time.

I feel like it's even more of a niche case than the Throw one, because you either need to write the template type manually or write a lambda (with explicit return type), no ? There's nothing "built-in" that allow that in the library yet, it will only exist once your ReturnCopy will be merged, right ?

I'm only hesitant because of the memory implications, but maybe it's a non-issue because mocks are short-lived, and users won't notice a big memory consumption increase.

I guess you're right. It's better to merge it, and maybe optimize later if we notice some issues with this dev. We could only mark some actions as "long-lived" and only keep them (like the action created by ReturnCopy) for example.

malcolmdavey commented 1 year ago

I think you can get the Return function in some cases to take a copy at the moment if you use an explicit value type as the template arg. Can't remember exactly. Would need to check the unit tests.