14

I recently struggled with a bug hard to find for me. I tried to pass a lambda to a function taking a std::function object. The lambda was capturing a noncopyable object.

I figured out, obviously some copy must happen in between all the passings. I came to this result because I always ended in an error: use of deleted function error.

Here is the code which produces this error:

void call_func(std::function<void()> func)
{
    func();
}

int main()
{
    std::fstream fs{"test.txt", std::fstream::out};
    auto lam = [fs = std::move(fs)] { const_cast<std::fstream&>(fs).close(); };
    call_func(lam);
    return 0;
}

I solved this by capseling the std::fstream object in an std::shared_ptr object. This is working fine, but I think there may be a more sexy way to do this.

I have two questions now:

  1. Why is this error raising up?
  2. My idea: I generate many fstream objects and lambdas in a for loop, and for each fstream there is one lambda writing to it. So the access to the fstream objects is only done by the lambdas. I want do this for some callback logic. Is there a more pretty way to this with lambdas like I tried?
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
JulianW
  • 897
  • 9
  • 23
  • 10
    /OT: Don't use `const_cast`, instead mark your lambda as `mutable`. – Rakete1111 Sep 15 '18 at 13:11
  • BTW, your `const_cast` is UB. You can't modify objects that were casted this way. The correct way to do this, as Rakete1111 pointed out, is to mark the object `mutable` – Not a real meerkat Sep 15 '18 at 13:18
  • Why I have to make it mutable? – JulianW Sep 15 '18 at 13:18
  • @CássioRenan What is UB here? The member variable is captured as non-`const`; it is only the generated `operator()` that is marked `const`. Casting away `const` from a `const` lvalue to a non-`const` object to modify said object is completely fine - and the entire point of `const_cast`. Sure, it's not good style here, I don't think, but it's not UB. Or if it is, why? – underscore_d Sep 17 '18 at 07:13
  • @JulianH because that is how to get an `operator()` that is not `const`-qualified and hence provides read-write access to the capture members of the lambda. – underscore_d Sep 17 '18 at 07:15
  • @underscore_d using `const_cast` on an object does not allow you to modify it. The "entire point" of `const_cast` is to allow you to use interfaces that are not const-correct, but that you know won't modify the objects passed to them. – Not a real meerkat Sep 17 '18 at 12:47
  • @CássioRenan It absolutely *does* allow modification, so long as the object was originally declared non-`const`. The only case in which UB arises is if the object was declared `const`, a non-`const` reference or pointer to it is formed via `const_cast`, and said reference/pointer is used to modify the object. Sure, it is probably bad design if you end up in a situation where you have a `const` reference/pointer to a declared non-`const` object and need to remove the `const`ness and modify the object, but that is a separate matter from the fact that doing so is totally legal/does not invoke UB. – underscore_d Sep 17 '18 at 16:50
  • @CássioRenan and [the other answer you cite](https://stackoverflow.com/questions/10498292/c-why-is-const-cast-evil/10498337#10498337) in yours agrees with me... "_In the first call, all is well. You can cast away the `const`ness of an object that is not really `const` and modify it fine."_ ...which is what OP was doing, since the captured `ostream` is not a `const` member, but instead the lambda's `operator()()` made it seem to be `const` – underscore_d Sep 17 '18 at 16:56
  • Does this answer your question? [How to create an std::function from a move-capturing lambda expression?](https://stackoverflow.com/questions/25421346/how-to-create-an-stdfunction-from-a-move-capturing-lambda-expression) – Ali Nov 16 '20 at 07:48

1 Answers1

12

The error happens because your lambda has non-copyable captures, making the lambda itself not copyable. std::function requires that the wrapped object be copy-constructible.

If you have control over call_func, make it a template:

template<typename T>
void call_func(T&& func)
{
    func();
}

int main()
{
    std::fstream fs{"test.txt", std::fstream::out};
    auto lam = [fs = std::move(fs)] { const_cast<std::fstream&>(fs).close(); };
    call_func(lam);
}

Following is my take on your idea in (2). Since std::function requires the wrapped object to be copy-constructible, we can make our own function wrapper that does not have this restriction:

#include <algorithm>
#include <fstream>
#include <iterator>
#include <utility>
#include <memory>
#include <sstream>
#include <vector>

template<typename T>
void call_func(T&& func) {
    func();
}

// All functors have a common base, so we will be able to store them in a single container.
struct baseFunctor {
    virtual void operator()()=0;
};

// The actual functor is as simple as it gets.
template<typename T>
class functor : public baseFunctor {
    T f;
public:
    template<typename U>
    functor(U&& f)
        :    f(std::forward<U>(f))
    {}
    void operator()() override {
        f();
    }
};

// In C++17 you don't need this: functor's default constructor can already infer T.
template<typename T>
auto makeNewFunctor(T&& v) {
    return std::unique_ptr<baseFunctor>(new functor<T>{std::forward<T>(v)});
}

int main() {
    // We need to store pointers instead of values, for the virtual function mechanism to behave correctly.
    std::vector<std::unique_ptr<baseFunctor>> functors;

    // Generate 10 functors writing to 10 different file streams
    std::generate_n(std::back_inserter(functors), 10, [](){
        static int i=0;
        std::ostringstream oss{"test"};
        oss << ++i << ".txt";
        std::fstream fs{oss.str(), std::fstream::out};
        return makeNewFunctor([fs = std::move(fs)] () mutable { fs.close(); });
    });

    // Execute the functors
    for (auto& functor : functors) {
        call_func(*functor);
    }
}

Note that the overhead from the virtual call is unavoidable: Since you need functors with different behavior stored in the same container, you essentially need polymorphic behavior one way or the other. So you either implement this polymorphism by hand, or use virtual. I prefer the latter.

Not a real meerkat
  • 5,604
  • 1
  • 24
  • 55
  • There's nothing wrong with a `mutable` lambda if it's known that it will run just once. For example, there's no risk in pushing a mutable lambda to a work queue. – François Andrieux Sep 15 '18 at 13:48
  • First, thanks a lot, but auto lam = [fs = std::move(fs)] () mutable { fs.close(); }; call_func(lam); does end in the same error for me. and your other solutions doesn't fit cause the std::fstream objects is longly out of scope when the lambdas are getting invoked. – JulianW Sep 15 '18 at 13:50
  • I want bind the std::fstream somehow to the lambda. – JulianW Sep 15 '18 at 13:54
  • @FrançoisAndrieux agreed. Still, it's better to not use them if there are other options available. Changes to code that uses mutable lambdas may cause unexpected behavior and hard to find bugs. For instance, `call_func` may in the future be changed to call the passed argument more than once under certain conditions: Note that the maintainer of that function may have no idea the argument being passed can not be called more than once. – Not a real meerkat Sep 15 '18 at 13:55
  • @JulianH which compiler (and version) are you using? [I can't reproduce this](http://coliru.stacked-crooked.com/a/a4cd61548f9aa3f2). – Not a real meerkat Sep 15 '18 at 13:57
  • @Cássio Renan Why I should move this object? Moving makes only sense to me if you save the lambda, but if you save the lambda, you will run into the error. My intention was, to save the object in the lambda, so I do not need to save it anywhere else. it's a little abuse I think. – JulianW Sep 15 '18 at 14:30
  • 1
    @JulianH I'm sorry, I didn't understand your question. You can move a copyable object into the lambda and you will be able to use it with `std::function` normally. e.g: just because you're moving the object, this does not mean the object must be not copyable. I edited my answer with my take on your idea in (2) – Not a real meerkat Sep 15 '18 at 14:54
  • You basically wrap the lambda with an object, like I did with shared_ptr. What is the benefit? I think, shared_ptr even doesn't have the virtual overhead. – JulianW Sep 15 '18 at 15:36
  • I haven't seen your implementation using shared_ptr, so I can't really know. – Not a real meerkat Sep 15 '18 at 15:38
  • It's just like: auto fs= std::make_shared("test.txt", std::fstream::out); and then move fs into the lambda. - no const_cast and no mutable needed. – JulianW Sep 15 '18 at 16:04
  • I see. In this case there will be still the overhead of std::function, which is basically the same (and, depending on the implementation, may be slower) as a virtual function call, so it's really trading apples for apples. Still, your way is probably simpler: I would stick with it. – Not a real meerkat Sep 15 '18 at 16:23
  • [There is no undefined behaviour in that `const_cast`.](https://stackoverflow.com/questions/52345009/passing-a-lambda-with-moved-capture-to-function#comment91669142_52345009) – underscore_d Sep 17 '18 at 17:01
  • 1
    @underscore_d You're correct. Edited answer to remove unnecessary changes in OP's code. – Not a real meerkat Sep 17 '18 at 17:07
  • yeah but anything I saw isn't looking like that's the cleanest way to do what I want to do. I only want to store the std::frstream in the lambda like a member of a class would save the std::fstream in the instance of the class. Nothing looked 100% correct. The argument: A lambda should always produces the same output is not correct, cause if you call a methods of a class you also may expect a different output for the same parameters, cause members may change, so this argument does not fit 100% to me. But it is also possible I understood something in lambdas complete wrong... – JulianW Sep 17 '18 at 17:37
  • @JulianH lambdas shouldn't be understood as classes, but as anonymous functions. That they internally generate a class is only an implementation detail. Frankly, if you want full, easy control on the behavior, you can simply write your own class. Lambdas aren't intended to be substitutes for proper classes. – Not a real meerkat Sep 17 '18 at 17:50
  • @CássioRenan Thanks! I presume you don't like the alternative, and I would think more idiomatic, option of using `mutable` instead? My initial reaction was that the `const_cast` seemed like worse style, in that it looks strange and unintuitive. However, after pondering some more, I guessthat the `const_cast` does have the arguable benefit that if other members were added, they couldn't be accidentally modified implicitly as with `mutable` and would instead need their own explicit `const_cast`s to enable that. – underscore_d Sep 17 '18 at 18:14
  • 1
    @underscore_d To be honest, I don't consider any of the two to be good style. But that's just me. There's just too many ways for subtle changes to introduce silent bugs. I cited one of those ways when responding to a comment by François, above. Also, `const_cast` has the added problem that if you actually do pass the lambda a const object, it will cause UB instead of causing a compiler error (as a `mutable` lambda would). – Not a real meerkat Sep 17 '18 at 18:26