Carry rvalue references through the nbx helper#113
Conversation
We do so with the public APIs so let's do it with the helper too
This makes the `std::moves` being performed in those functors look a lot less dangerous
loganharbour
left a comment
loganharbour
left a comment
There was a problem hiding this comment.
👍🏼 looks good to me
not sure why we didn't do this previously...
loganharbour
left a comment
loganharbour
left a comment
There was a problem hiding this comment.
Shouldn't we still test the reference case? I suspect folks have implemented the lambda as non auto and don't pass in an rvalue ref
|
if the action functor takes the data as an lvalue reference then with the change here, their code fails to compile, hence why I had to change the unit tests here. But if their action functor takes the data as a const lvalue reference then their code will still work. In MOOSE when compiling the contact module, there was only one action functor I had to change, and it was in the framework |
loganharbour
left a comment
loganharbour
left a comment
There was a problem hiding this comment.
We should update the doxygen to reflect this; I was just working with @grmnptr and he was confused because the documentation explicitly says you get a const ref to the received containers. Not very intuitive when you're trying to use move semantics
And it looks we have TIMPI-internal test coverage on that still? If so then I'm happy here; I never intended an action functor to be able to take a non-const lvalue reference, only a const one. |
|
Yes, that's right, we still have examples in the TIMPI test suite of action functors taking const lvalue references |
|
@roystgnr I'm going to wait until you approve before merging |
roystgnr
left a comment
roystgnr
left a comment
There was a problem hiding this comment.
I always hate having to change a test case or user code, but we really did never intend to support non-const lvalue references there.
The TIMPI docs always said we require action functors to take *const* lvalue references, but we seem to have never actually enforced that until we added the rvalue reference support. Refs libMesh/TIMPI#113 , where Alex describes making this change but I guess didn't actually push this change? This fixes libMesh/libmesh#3500 for me, and it'll be needed for the next libMesh submodule update too. Refs #000
4 participants
We do so with the public APIs so let's do it with the helper too