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

Oss 7 copy return for return by ref methods #310

Closed FranckRJ closed 2 months ago

FranckRJ commented 1 year ago

New PR because I can't reopen the old one : https://github.com/eranpeer/FakeIt/pull/280

coveralls commented 1 year ago

Coverage Status

Coverage: 99.925% (+0.0002%) from 99.925% when pulling 36ad3e6a6ca4520714b6a927ae0196e773587fe0 on iress:OSS-7-copy-return-for-return-by-ref-methods into 3c36d1a1a3ff3f96dfa0e95f1d962bbd47fd5552 on eranpeer:dev.

malcolmdavey commented 1 year ago

Any change of getting this one in? Is there anything outstanding.

malcolmdavey commented 1 year ago

For issue https://github.com/eranpeer/FakeIt/issues/258

FranckRJ commented 11 months ago

Sorry for the late reply. In the original PR I made this comment: https://github.com/eranpeer/FakeIt/pull/280#issuecomment-1303769386

The first two points aren't relevant anymore, but if you could answer the third one it would be nice.

malcolmdavey commented 11 months ago

Sorry for the late reply. In the original PR I made this comment: #280 (comment)

The first two points aren't relevant anymore, but if you could answer the third one it would be nice.

Return(T&& t): from what I understand it's to fix an issue when passing rvalues to Return and the return type of the mocked function is a reference, but I have several questions about it, that makes me not too confident about merging it in its current state: why using a new template parameter instead of R like others Return ? I don't think it's a bad idea per se but it's not how others Return a written and change a bit how the method work, I think it may be better to keep it consistent. why do you check that T is copy constructible ? What about move-only types ?

  1. Can't remember about the first case, why I specifically changed it. Will experiment (though I think with out the VS projects might take me a while to get it all running/testing etc)
  2. Also I think for the AlwaysReturn, we can't have move-only types. But we could with the Return I guess. As longs as we are happy with that.
FranckRJ commented 2 months ago

Superseded by #332.