ice icon indicating copy to clipboard operation
ice copied to clipboard

Add move ctor and assignment operator to InputStream

Open bernardnormier opened this issue 1 year ago • 8 comments

This PR adds a move ctor and assignment operator to InputStream and uses them in a few spots.

Move ctor / move-assignment operator is more idiomatic than swap in modern C++. Unfortunately we rely on swap keeping "instance" as is which makes this PR not quite correct.

bernardnormier avatar Apr 30 '24 18:04 bernardnormier

I've merged my changes to your branch. The issue is that I can't move the stream into the lambda. The following doesn't work:

[self, outAsync = outAsync->shared_from_this(), stream = std::move(is), requestId, dispatchCount]() mutable
{
    if (self->sentAsync(outAsync.get()))
    {
        self->dispatchAll(stream, requestId, dispatchCount);
    }
}

The std::function is not copy-constructable:

src/Ice/CollocatedRequestHandler.cpp:166:43:   required from here
/usr/include/c++/11/bits/std_function.h:439:69: error: static assertion failed: std::function target must be copy-constructible
  439 |           static_assert(is_copy_constructible<__decay_t<_Functor>>::value,
      |                                                                     ^~~~

Do you see a way to make this work?

bentoi avatar May 01 '24 07:05 bentoi

Do you see a way to make this work?

Did you try to move the function?

// test.cpp
#include <iostream>
#include <memory>

template <typename Func>
void processLambda(Func func)
{
    func(); // Invoke the lambda
}

int main()
{
    auto uniquePtr = std::make_unique<int>(10);

    auto lambda = [ptr = std::move(uniquePtr)]() mutable
    {
        std::cout << "Value inside unique_ptr: " << *ptr << std::endl;
    };

    processLambda(std::move(lambda)); 

    return 0;
}

This seems to work, only tested with clang

clang++ test.cpp -o test --std=c++20
./test                              
Value inside unique_ptr: 10

pepone avatar May 01 '24 08:05 pepone

I'll give it a try.

bernardnormier avatar May 01 '24 13:05 bernardnormier

I tried and it doesn't work. The capture of a lambda must be copyable, even if we never copy it.

So either we somehow pass this InputStream as a parameter to the function (and the std::function<void()> becomes something like std::function<void(InputStream)> or we use a heap-allocated InputStream (shared_ptr) like Benoit did.

bernardnormier avatar May 01 '24 14:05 bernardnormier

I tried and it doesn't work. The capture of a lambda must be copyable, even if we never copy it.

I works when you use a template parameter for passing the lambda, as in the example I posted.

pepone avatar May 01 '24 14:05 pepone

I tried and it doesn't work. The capture of a lambda must be copyable, even if we never copy it.

I works when you use a template parameter for passing the lambda, as in the example I posted.

I believe this works because you don't use a std::function at all. This doesn't seem practical with our ThreadPool code.

bernardnormier avatar May 01 '24 14:05 bernardnormier

If it's not used anywhere, do we give up on this PR?

This PR updates ConnectionI to use it in one spot. I am in favor of merging this PR so it can be used more.

bernardnormier avatar May 06 '24 14:05 bernardnormier

I think it's good to have and we should use it where we can.

externl avatar May 06 '24 15:05 externl