Add ReturnAndSet() & AlwaysReturnAndSet() functions to assign output args

[hedayat]

Add new functions to MethodStubbingProgress, so that output args can be set in addition to return value for a function

[coveralls]

Coverage increased (+0.001%) to 99.924% when pulling 1d5abd2edc1b78835df6b62072e3cca9be49245e on hedayat:add-arg-assigner into 80a446b8d9a8740a58e09b004d5ac0c3094400bb on eranpeer:master.

[FranckRJ]

@hedayat Can you please fix the build and provide some tests ?

Here are the logs : https://travis-ci.org/github/eranpeer/FakeIt/jobs/720171959

FakeIt is a C++11 library, so you can't use C++14 / C++17 features.

[hedayat]

I'll try to see what I can do; but not right now. Can take some time.

I should check closely to see if I can implement the same in C++11, if not, it is probably acceptable to protect these with conditionals so they are only available if compiler can support them.

But I'm afraid I can't do anything for at least about 2 months. So, if somebody is willing to create something on top of this PR, that'd be great.

[hedayat]

OK, now it builds successfully (although still uses a small C++14 extension but apparently all compilers are happy with it).

[malcolmdavey]

Suggest you add some unit tests, and also add to the docs with examples

[malcolmdavey]

Also wondering if we will be able to set Return and use Set for the same mock invocation (similar to googlemock DoAll() )

[hedayat]

Suggest you add some unit tests, and also add to the docs with examples

OK, let's see what I can come up with. But I don't promise much.

Also wondering if we will be able to set Return and use Set for the same mock invocation (similar to googlemock DoAll() )

Hmmmm, not sure if I understand what you want, but using Set() you can both set return value and params at the same time so there is no need to call Return too.

[hedayat]

OK, now there are tests.

About the docs, I didn't find how should I send PR for docs, so I edited the wiki hoping that it'll go for review; but the changes immediately published to public. I don't know what I should have done so that it won't appear publicly now. Please fix it as you see appropriate.

[malcolmdavey]

Also wondering if we will be able to set Return and use Set for the same mock invocation (similar to googlemock DoAll() )

Hmmmm, not sure if I understand what you want, but using Set() you can both set return value and params at the same time so there is no need to call Return too.

Okay - now having a closer look at the code (and the examples - thanks) understand it a bit better. Can see that you are setting the return value and all the arguments in one step. (Was assuming there would be a separate step/method for each arg).

Wondering how this would handle the case where some of the arguments are input parameters, instead of a non-const ref or pointer - are they just ignored, which may make the Set() misleading.

[malcolmdavey]

OK, now there are tests.

About the docs, I didn't find how should I send PR for docs, so I edited the wiki hoping that it'll go for review; but the changes immediately published to public. I don't know what I should have done so that it won't appear publicly now. Please fix it as you see appropriate.

Thanks - that helps.

[hedayat]

Okay - now having a closer look at the code (and the examples - thanks) understand it a bit better. Can see that you are setting the return value and all the arguments in one step. (Was assuming there would be a separate step/method for each arg).

Well, I was thinking that maybe a better naming will help. Maybe ReturnAndSet() or SetAndReturn() (or maybe using Assign instead of Set in whatever name) are better? (With some names it might be even better to move the return value to the end instead of being the first parameter)

Wondering how this would handle the case where some of the arguments are input parameters, instead of a non-const ref or pointer - are they just ignored, which may make the Set() misleading.

Well, it actually does 'set' them, but since they are input parameters it naturally doesn't do anything; so they are effectively ignored. Currently it is assumed that the developer knows that values for input parameters are just ignored. :P

I wonder if anything better can be done and if it would be less confusing.

[malcolmdavey]

ReturnAndSet

ReturnandSet (or ...Assign? ) sounds best for it's current format.

Wonder if some extra wrapper can be used - something like this - so you can set the argument number.

.ReturnAndSet( return-value, SetArg<1>(arg1-value), SetArg<4>(arg4-value))

[hedayat]

Well, I guess it is possible to receive values only for reference/pointer parameters, but I'm not sure if it is a good decision. Actually, I was thinking that even some apparently input variables can have outside effects. So, we won't know if assigning a value to a non-pointer/non-reference variable has any effects or not.

Also, I noted that the current implementation doesn't work for smart pointers, which might be fixed too.

I liked the idea of specifying the argument number (or any other way to let the user to select which parameters to assign). Hmmm, but it's better for simple cases to be simple to express, so that using Do() won't be easier than this new function :)

[hedayat]

I'm going to sleep now, but I was wondering if we can come up with something like one of these in addition to the current simple format:

. ReturnAndSet(return-value, {1, v1}, {2, v2}, ...)
ReturnAndSet(return-value, {1 => v1, 2 => v2, ...})

[hedayat]

Well, it seems that this can be made to work:

.ReturnAndSet(return-value, _1 <= v1, _2 <= v2, ....)

I like it. I got it working but needs some work to be really usable.

[hedayat]

OK, now I renamed Set/AlwaysSet functions to ReturnAndSet/AlwaysReturnAndSet functions. Also, I added a new style to selectively assign values to function arguemtns. The new style is this:

.RetrunAndSet(return-value, _2 <= val2, _3 <= val3)

[malcolmdavey]

Hi @FranckRJ . I'm guessing you have permission to merge. Not sure who else does who is still active.

Do you think this one is ready?

[FranckRJ]

I will look at it this weekend.

[FranckRJ]

There are some parts of the code that use features of C++14 (initialized lambda captures).

[vals_tuple = ArgumentsTuple<valuelist...>{
    std::forward<valuelist>(arg_vals)...}
] (typename fakeit::test_arg<arglist>::type...args)
{
    helper::ParamWalker<sizeof...(valuelist)>::Assign(vals_tuple,
        std::forward<arglist>(args)...);
};`

I'm not sure what's the best solution for making the code compatible with C++11.

[hedayat]

Yeah, but it was supported by all CI compilers in C++11 mode, so I thought it'd be practically OK. If not, I guess the easiest solution is to provide this feature for C++14 and above only.

[FranckRJ]

It compiles but it shows a warning. Maybe it's not a big deal and we can just try to disable the warning, maybe we should try to make the code C++11-compatible (I guess it's technically possible to create an inner class and return an instance of it ? Not 100% sure), maybe we should make this feature C++14-only (I don't want to). I don't know.

I may look into the inner-class solution this weekend, if it works I'll update the feature.

[malcolmdavey]

I'm also in favour of trying to keep the library moving forward as much as possible. Have a lot of small ideas, and a bigger one to solve that known issue. (just need to grab enough time to complete them properly)

[FranckRJ]

I've done it in #264, I think I'll release the 2.1.0 this week.

[hedayat]

Great (I'd probably also keep the C++14 implementation around so that C++11 ones would be removed when the minimum requirement increased; although it'd make the code messy :P)

[FranckRJ]

For the auto return type i think have it enabled for C++14 is a good idea because it remove one level of indirection so it provide better perfs (even if it's probably negligible). For the lambda / local-class I don't think there is any performance benefit for one or the other, so I'll let it like that.

[hedayat]

Yeah, that's fine. I was considering cleaner code in future rather than any performance benefits for this case. But I'm fine with this. When migrated to future versions of standard, someone can clean this up. :)