Move only callables

[omartijn]

This PR:

• bumps c++-version to 23
• uses std::move_only_function for one-shot callbacks
• sprinkles some mutable for lambdas moving captures out
• disabled clang-format

The lambdas made mutable were for different reasons. In a few spots, there were lambdas capturing something and then using std::move() to move the capture out on invocation. That only works correctly if the lambdas is mutable, Without mutable, the capture is const and you'll get a const T&&, which is not what you want.

Not all callbacks are converted to std::move_only_function either. I only did that for "one-shot callbacks". In contrast to std::function, the invocation operator of std::move_only_function is not const-qualified. It may change internal state (and not be safe for repeated invocation).

Clang-format I have disabled because there were no formatting rules, causing default rules to be automatically applied whenever I save a file. I do think it could be a good idea to settle on a particular formatting at some point.

Note that I didn't change any of the tests. That's not necessary, because it's OK to move an std::function into an std::move_only_function (but not the other way around). This shows that the changes are an ABI-break but not an API-break.

[sangelovic]

Thanks for the PR. A few comments:

• None of the lambdas marked mutable actually std::moves any of its captures out. If it does, it's a capture by non-const reference, not by value. I do not see a single reason to mark them mutable. Or am I missing something?
• Some callbacks you changed into std::move_only_function are repeated callbacks. Two of them (signal_handler and message_handler) are called potentially repeatedly. Only async_reply_handler is called once always. Was this intentional (it contradicts your statement about changing one-shot callbacks only) or inadvertent? Shouldn't we change async_reply_handler only?

But I would perfectly imagine clients may need move-only semantics for other callback types, too, not only async_reply_handler. As you said in the issue, it allows them to register move-only lambdas (i.e., lambdas with move-only captures, like those of type std::unique_ptr or std::promise, for example) via sdbus-c++ API. Doesn't it make sense to you as well? Or you only need async_reply_handler to be move-only in your project?

[omartijn]

Thanks for the PR. A few comments:

* None of the lambdas marked `mutable` actually `std::move`s any of its captures out. If it does, it's a capture by non-const reference, not by value. I do not see a single reason to mark them `mutable`. Or am I missing something?

Yes, this is a good question. The reason is that without mutable, all captures are automatically const inside the lambda body. It's at this spot where c++ broke with the behaviour of mutable by default (which I think is a good thing). Since std::move_only_function does not have a const-qualified overloaded invocation operator, this would fail. Take the following very simple example:

#include <functional>
#include <iostream>

int main() {
    std::move_only_function<void()> callback{
        [] { std::cout << "Invoked!" << std::endl; }};

    auto wrapper = [callback = std::move(callback)] { callback(); };

    wrapper();
    return 0;
}

This does not compile because it tries to invoke the captured callback on a const reference. Making the lambda mutable fixes this.

* Some callbacks you changed into `std::move_only_function` are repeated callbacks. Two of them (`signal_handler` and `message_handler`) are called potentially repeatedly. Only `async_reply_handler` is called once always. Was this intentional (it contradicts your statement about changing one-shot callbacks only) or inadvertent? Shouldn't we change `async_reply_handler` only?

I think you're right! I may have done too much there. I'll change that back. That's indeed unintentional.

But I would perfectly imagine clients may need move-only semantics for other callback types, too, not only async_reply_handler. As you said in the issue, it allows them to register move-only lambdas (i.e., lambdas with move-only captures, like those of type std::unique_ptr or std::promise, for example) via sdbus-c++ API. Doesn't it make sense to you as well? Or you only need async_reply_handler to be move-only in your project?

There is a reason for only doing it for one-shot callbacks (which I mistakenly bungled up in my PR as you rightly pointed out!). The reason is that std::function::operator() is const-qualified, meaning it cannot make changes to itself when being invoked. That implies it's safe to call repeatedly. An std::move_only_function can only be invoked on a non-const instance.

[sangelovic]

Thanks for more detailed elaboration. To the first point, I think I now understand, you want to achieve two things:

• by changing to async_reply_handler to std::move_only_function, you want enable that your callables (coming from boost::asio, for example) can capture move-only types, since std::function requires copyability and that would fail the compilation.
• however, for that to work, you also needed to change lambda in AsyncMethodInvoker::makeAsyncReplyHandler to be mutable => your callable is moved into that lambda as callback capture, and that lambda -- turned into std::move_only_function -- can be moved further, transitively moving its callback capture. Otherwise the capture (your callable) would be const and that the move would turn into copy, failing compilation.

I was first confused by you mentioning that captures are moved, but I didn't see any explicit std::move in the lambda expression. Of course, std::move can happen also implicitly, when the enclosing lambda (the closure object) is itself moved. All OK now :-)

What I still don't quite get is why not make the other callback types std::move_only_function, too. It brings all the advantages, like ability to support move-only callables, thus move-only captures in closures, which is impossible now with std::function. And having non-const operator() in std::move_only_function can be also seen as an advantage -- imagine a client lambda for signal callback captures a counter by value and increments it every time a D-Bus signal arrives. Or maintains a cache internally... these need to be non-const captures. So we get more generic with this -- we allow more freedom to the client. Additionally, client may make operator() in std::move_only_function to be const, by simply marking the callable type as const in std::move_only_function template argument. I discussed this also with GitHub Copilot, which came with the same conclusions. Why should we tie usage of std::function vs. std::move_only_function to the concept of one-shot vs. repeatable callbacks? Can you elaborate further?

Thanks a lot.

[omartijn]

I was totally unaware of the option that you can const-qualify the signature of the callback for an std::move_only_function and then having the invocation operator become const-qualified itself. That's a neat thing!

I'm OK with just using std::move_only_function for everything, but I think the user isn't the one specifying the signature here - we take any callback and store it in an std::move_only_function with a signature of our choosing. But we can simply ensure all our lambda wrappers are mutable and we can invoke them. Reason I wanted to do it this way is that's what I'm used to with Boost.asio (their handlers can invoked only once and they're not const-qualified). That's not a strong reason.

[omartijn]

OK, after your latest comment and realizing that you can put qualifiers on the type in the std::move_only_function, I realized we can make this much cleaner.

• We can make all the callbacks std::move_only_function
• The ones used internally can have const (they don't use mutable state), making them invocable from const member functions
• The one-shot actions are qualified with && - so they can only be invoked on an rvalue. This clearly signals that they cannot be invoked repeatedly (as you need to move them to call them). A regular lambda or function can of course be implicitly converted to this (so old code remains functional).
• The builder pattern is converted so it only uses rvalues - since you never hold them

I also took the liberty of converting some static_asserts in the code to constraints. That's in the latest commit and not strictly related to the other changes, so that could be easily moved to another PR if you prefer that.

[sangelovic]

@omartijn Thanks for your additional thoughts and work. I will look at it and come back. In the meantime I realized we don't have pipelines enabled on feat/release-v3 branch, plus some new stuff appeared on master, so I took the liberty to rebase your branch on latest feat/release-v3 and force-push, hope that is fine for you...

[omartijn]

Nice! I'm definitely "team rebase", so not an issue. Saw some issues in the tests flagged by clang-tidy so fixed those too. Also disabled a few tests that probably should be fixed but they were pre-existing (likely not flagged with an older clang-tidy). Also: The tests run with a very out-dated c++ library it seems so that doesn't have std::move_only_function.

[sangelovic]

OK, I also realized there was a bug with running ctest in CI jobs :-) So since you are "team rebase", I did one again :-) And while I started to work on clang-tidy issues, I see you've fixed them already -- great! I did a few changes directly to your commits while rebasing, or adding new commits of mine:

• I removed the new clang-tidy suppressions, and fixed one more finding in my clang-tidy run (I have v19 locally). I'd like to see real issues instead of disabling them upfront.
• Marking the builder class function with && is good idea -- I had this in my mind a few years ago, but totally forgot about this improvement.
• I cherry-picked your concepts commit (which is again a good thing now that we will definitely drop C++17 compatibility here, and there are more spots where we can use concepts) away into a separate MR: Convert some static_asserts to concept constraints #534. We will rebase that MR once we merge this MR.
• I removed mutable from two internal lambdas provided to invokeHandlerAndCatchErrors() and had invokeHandlerAndCatchErrors() take them by const ref. I think this is simpler and we don't need move semantics there.
• A few smaller changes, like using higher-level API for addVTable in tests, which I find more concise and expressive, and capturing std::promise directly in yet another place in the lib.

Feel free to review. From my side I think we are good to go. I like your separation of callbacks and additional marking with const or &&, I think this is more idiomatic and self-revealing to users, and enables more things as well. Let's see where this will get us.

[sangelovic]

I must admit I'm still a bit hesitant about const qualifier in server-side callbacks. While it allows to use const functions, it may also -- confusingly so -- imply that these shall be pure, immutable functions. However, sending a command to the D-Bus service, or sending property-set message, implies that the service may do a mutable operation.

Furthermore, user can use const function also without const qualifier, by calling it indirectly through lambda and capturing this.

Is there any issue you foresee (except that you are used to it with boost::asio) for sdbus-c++ users if we removed const qualifier from the three server-side callback signatures? If we remove const there, we can use mutable lambdas in higher-level API code, which means users can use std::move_only_function not only for lower-level API, but also for higher-level API, which makes it more consistent (and he may mark that higher-level callback as const, since that's in his hands).

[omartijn]

No - the const there is only to keep the API the same. Since the std::function::operator() is always const-qualified, you had member functions invoking those also as const and I wanted to keep the API the same. But it makes sense to change that - it conceptually changes something so it makes sense to change that as well.