maidsafe-archive / MaidSafe-RUDP

Implementation of Reliable UDP
Other
95 stars 51 forks source link

Enabled some tests #63

Closed inetic closed 9 years ago

inetic commented 9 years ago

Few tests are still missing and there is a lot of commented out code which is either an old code that has been replaced, a missing test or test for functionality no longer present in the new API (one without validation messages).

dirvine commented 9 years ago

Thanks Peter we will get this merged now. I think think week we will try and get routing and rudp routing_v2 branches added back to Qa again. Great stuff thanks again. I do question the && use in some places though, but its an ongoing debate as yet unresolved so we will see :-)

inetic commented 9 years ago

David, yes, if you mean using rvalue reference in the public ManagedConnection::* methods then I was wondering why they were added as well. My understanding is that one should use rvalue reference if it adds a value, but in this case it doesn't because we're making copies of the token anyway (as a lambda capture). Also, I'm now fixing the clang build and to do so, it seems, I need to either put std::decay at places or just replace the rvalue reference with const refs. I like the later option better.

dirvine commented 9 years ago

Agreed Peter, I wonder if a pass by value idiom works here. Then if these are posted away etc. the client can call std::move if they want to declare an xvalue or even have a prvalue constructed right there.

I like that as it allows the user of the API to decide whether to move or copy and as we post then we probably want full control over that parameter from the entry to the method. This stuff is all up for debate though, but I like the pass by value idiom in places whare we consume a value (shove it on a thread or yield etc.) . The recent Eric Neibler (https://www.youtube.com/watch?v=zgOF4NrQllo) video is pretty good at outlining the mentality there. As I aid though its my opinion just now and not policy or anything. I just think its clean and worst case a copy as opposed to undefined behaviour or races when threaded. I am not 100% as you can tell though ;-)

inetic commented 9 years ago

The pass by value would have worked if c++ supported move semantic in lambda capture I think. Let's count how many copies need to be done in each idiom when user passes either rvalue or not:

Pass-by-value: if user passes rvalue, then one copy in arguments is elided but then we need to copy it anyway to capture it in the lambda for posting. If non-rvalue is passed then one copy is made in arguments and then again one copy to capture it in the lambda, so we have (rvalue:1, non-rvalue: 2) copies. If move semantic for lambda capture was supported, we would have (rvalue: 0, non-rvalue: 1)

Allow only rvalue: this is the same as above but only allow user to pass rvalue and lose content if its argument.. So (rvalue: 1, non-rvalue: nil).

Pass by const ref: if user passes rvalue, it's treated as const ref, that is, no copy is made during argument initialization and one copy is made in lambda capture, so we have (rvalue: 1, non-rvalue:1).

Perfect forwarding: since, again we need to make one copy to capture in lambda, this will be the same as above: (rvalue:1, non-rvalue: 1).

At least thats my understanding, though I'm also new to this so I might be wrong.

inetic commented 9 years ago

Also note, if move semantics for lambda capture was supported, then the perfect forwarding case would do (rvalue: 0, non-rvalue:1) copies as well as the copy-by-value idiom and since copy-by-value is easier to implemet and results in fewer template instances it is preferred. This same argument tells me to prefer pass-by-const-ref over perfect forwarding now when we cant move to lambda capture.

dirvine commented 9 years ago

Yes not having move aware lambdas really is a pain. I feel when we get msvc 2015 it will be enough to allow a switch to c++14 cross platform. This will save a ton of code, especially templates with generic lambdas at last being available. Until then I agree. +1