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 sharded<>::invoke_on_all() const overload #2278

Open
xemul opened this issue May 31, 2024 · 1 comment
Open

Add sharded<>::invoke_on_all() const overload #2278

xemul opened this issue May 31, 2024 · 1 comment

Comments

@xemul
Copy link
Contributor

xemul commented May 31, 2024

The map-reduce-s have it, why not have for invoke_on_all()? However, it looks like it's going to break compilation

Consider this

#include <fmt/core.h>

template <typename T>
class wrapper {
    T _x;

public:
    wrapper() : _x() {}

    template <typename Fn>
    requires std::invocable<Fn, T&>
    void call(Fn fn) {
        fmt::print("non-const call\n");
        fn(_x);
    }

    template <typename Fn>
    requires std::invocable<Fn, const T&>
    void call(Fn fn) const {
        fmt::print("const call\n");
        fn(_x);
    }
};

class foo {
public:
    void fn() {
        fmt::print("non-const fn\n");
    }

    void fn() const {
        fmt::print("const fn\n");
    }
};

int main() {
    wrapper<foo> w;

    w.call([] (auto& x) {
        x.fn();
    });
}

Note that there are two wrapper::call() overloads -- const and non-const one, just like we want for sharded::invoke_on_all.
If compiled and run we'll see

non-const call
non-const fn

which is expected -- no const is involved at all. However, if removing the foo::fn() const overload, the whole thing will fail to compile with

x.cc:36:9: error: 'this' argument to member function 'fn' has type 'const foo', but function is not marked const
        x.fn();

Respectively, adding sharded::invoke_on_all() const will fail compilation of any non-const calls to sharded<S>::invoke_on_all(&S::method) for S's that don't have S::method() const-s

@xemul
Copy link
Contributor Author

xemul commented May 31, 2024

Cc @tchaikov

xemul added a commit to xemul/seastar that referenced this issue May 31, 2024
There are several options what invoke_on_...() can try to invoke. So far
only non-const and non-mutable callables are in use and it works.

If calling it with mutable lambdas, it used to fail compilation, but
previous patch fixed it, so here's the test.

It also makes sense to support calling it on const sharded<> object
(with the lambda accepting const service reference), but it's not that
simple (see scylladb#2278)

Signed-off-by: Pavel Emelyanov <[email protected]>
xemul added a commit to xemul/seastar that referenced this issue Jun 3, 2024
There are several options what invoke_on_...() can try to invoke. So far
only non-const and non-mutable callables are in use and it works.

If calling it with mutable lambdas, it used to fail compilation, but
previous patch fixed it, so here's the test.

It also makes sense to support calling it on const sharded<> object
(with the lambda accepting const service reference), but it's not that
simple (see scylladb#2278)

Signed-off-by: Pavel Emelyanov <[email protected]>
tchaikov pushed a commit to tchaikov/seastar that referenced this issue Jun 5, 2024
There are several options what invoke_on_...() can try to invoke. So far
only non-const and non-mutable callables are in use and it works.

If calling it with mutable lambdas, it used to fail compilation, but
previous patch fixed it, so here's the test.

It also makes sense to support calling it on const sharded<> object
(with the lambda accepting const service reference), but it's not that
simple (see scylladb#2278)

Signed-off-by: Pavel Emelyanov <[email protected]>
tchaikov pushed a commit to tchaikov/seastar that referenced this issue Jun 6, 2024
There are several options what invoke_on_...() can try to invoke. So far
only non-const and non-mutable callables are in use and it works.

If calling it with mutable lambdas, it used to fail compilation, but
previous patch fixed it, so here's the test.

It also makes sense to support calling it on const sharded<> object
(with the lambda accepting const service reference), but it's not that
simple (see scylladb#2278)

Signed-off-by: Pavel Emelyanov <[email protected]>
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

No branches or pull requests

1 participant