Take copy of return value if explicit by value used, when mocking return for return by ref or const ref method.

[malcolmdavey]

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

[FranckRJ]

Initializers in lambda captures is a feature of C++14, it should be changed to something that compile on strict C++11, like local classes if there's no other solution. You don't necessarily have to do it, I can do it myself when I'll merge this PR (this week I hope).

[coveralls]

Coverage increased (+0.0002%) to 99.925% when pulling 36ad3e6a6ca4520714b6a927ae0196e773587fe0 on iress:OSS-7-copy-return-for-return-by-ref-methods into 9ccbf9a8e29a59d795178d237d2dccd9734beea6 on eranpeer:dev-2.3.1.

[malcolmdavey]

Actually the move there is optional, and just an optimisation. It's a shared_ptr so it can be copied fine.

[FranckRJ]

I think that the feature may be a bit obscure (I mean, having to specialize the function call). It's not really obvious what it does, it feel a bit like a hack. Maybe having a separate function would be a better idea, what do you think ?

[malcolmdavey]

Yeah - initially I was thinking of a "ReturnCopy" function, but then found this also worked - wasn't sure if it was deliberate or an accident. Am happy to make a "ReturnCopy" and a "AlwaysReturnCopy" instead.

[malcolmdavey]

Hi @FranckRJ . Any time to review?

[FranckRJ]

I'll try to look at it next week.

[malcolmdavey]

Hi @FranckRJ. Sorry to bother you again. Is this one reasonable to go ahead?

[malcolmdavey]

Hi @eranpeer . Are you still interested in participating in this project?

[malcolmdavey]

Hi @FranckRJ . Do you have an idea if you will be able to get to this? Or are there others who might be able to. (FYI - I think this one is lower priority than the Verify of reference parameters issue, but hadn't started that yet..)

[FranckRJ]

Sorry for the late reply, last time I checked this PR I was a bit confused about it and didn't know what to do, because it contains multiple changes and I have a different opinion on what to do for all of them.

• Extending lambda lifetime: a needed fix, I would like to merge that in 2.3.1
• ReturnCopy: a good new feature, but as a new feature I think it should better be merged in 2.4.0
• 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 ?

[FranckRJ]

So ideally I would like that these 3 changes have their own PR, I understand that it's a bit annoying to do so I can to it myself if you want.

[malcolmdavey]

Hi @FranckRJ . Sorry, not sure what happened with the PR. It does seem to contain changes from other PR's which may have been merged already. I'll try to fix it up..

[FranckRJ]

I aim to release the 2.3.1 next week, I would like to have the extension of lambda's lifetime merged in. Do you mind if I do the PR myself if you don't have the time to do it ?

[malcolmdavey]

Yeah - that's fine. Not sure I'll have time in the next week or so.

[FranckRJ]

I'll remove this from 2.3.1 milestone as I created #301 for the lifetime fix.

[FranckRJ]

Sorry, I deleted the branch dev-2.3.1 after merging it to master, I didn't know it will close the PR. I cannot rebase it to dev-2.4.0 nor re-open it.

[malcolmdavey]

No worries, I can fix it up.