Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add move ctor and assignment operator to InputStream #2092

Merged
merged 7 commits into from
May 15, 2024

Conversation

bernardnormier
Copy link
Member

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 bernardnormier marked this pull request as draft April 30, 2024 18:46
@bentoi
Copy link
Member

bentoi commented May 1, 2024

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?

@pepone
Copy link
Member

pepone commented May 1, 2024

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

@bernardnormier
Copy link
Member Author

I'll give it a try.

Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I guess that we need to test that it's usable for our use cases. I agree this is preferable to the swap calls.

@bernardnormier
Copy link
Member Author

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 bernardnormier requested a review from pepone May 1, 2024 14:02
@bernardnormier bernardnormier marked this pull request as ready for review May 1, 2024 14:11
@pepone
Copy link
Member

pepone commented May 1, 2024

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.

@bernardnormier
Copy link
Member Author

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.

Copy link
Member

@bentoi bentoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bernardnormier
Copy link
Member Author

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.

@externl
Copy link
Member

externl commented May 6, 2024

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

@bernardnormier bernardnormier requested a review from bentoi May 9, 2024 13:56
@bernardnormier bernardnormier merged commit e349f8b into zeroc-ice:main May 15, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants