C higher order functions: different results when declaring multiple functions-CodePudding

I was plying with higher order functions, and I started getting different results when I created two instances and assigning them to variables.

I reduced the issue to the following example:

#include <iostream>

using ColorGetter = int(*)(void);

auto paint(const ColorGetter &f)
{
    return [&]() { return f(); };
}

int colorA() { return 10; }

int colorB() { return 20; }

int main()
{
    auto painter1 = paint(colorA);
    auto painter2 = paint(colorB);
    std::cout << "painter 1 : " << painter1() << "\n";
    std::cout << "painter 2 : " << painter2() << "\n";

    auto p1 = [] () { return colorA(); };
    auto p2 = [] () { return colorB(); };
    std::cout << "p 1 : " << p1() << "\n";
    std::cout << "p 2 : " << p2() << "\n";
}

My expectation was to get 10, followed by 20 from both sequences. Instead, depending on the compiler, I get:

➜  tmp clang  -13 -o out.gcc wrong.cpp&& ./out.gcc       
painter 1 : 10
painter 2 : 20
p 1 : 10
p 2 : 20
➜  tmp g  -11 -o out.gcc wrong.cpp && ./out.gcc
painter 1 : 20
painter 2 : 20
p 1 : 10
p 2 : 20

Is there something fundamentally wrong with the above code? I get no compiler warnings or clang-tidy issues, at least with my current settings.

CodePudding user response：

Let's look at this code:

auto paint(const ColorGetter &f)
{
    return [&]() { return f(); };
}

The f parameter only exists for the lifetime of the function paint. Your lambda function captures it by reference (that's the [&] bit), and so your lambda capture is referencing a variable (the reference f) that no longer exists after the function ends. As a result, the object you're returning holds a dangling reference to the function. You're seeing undefined behavior here, which is why the result varies across compilers.

To fix this, change the code to read

auto paint(const ColorGetter &f)
{
    return [=]() { return f(); };
}

This will cause the lambda capture to make a copy of the value stored in f (the pointer to the function in question), which will outlive the paint function.

CodePudding user response：

You aren't keeping track of lifetime.

auto paint(const ColorGetter &f)
{
  return [&]() { return f(); };
}

this captures a reference to f. You should almost NEVER use [&] when the lambda outlives the scope it is created in.

The f is:

auto painter1 = paint(colorA);

a temporary pointer created on this line. It is discarded at the end of the statement.

So your code, when it does f(), exhibits undefined behavior -- you are following a dangling reference.

The easy fixes include:

auto paint(const ColorGetter &f)
{
  return [=]() { return f(); };
}

I would also get rid of the reference-to-pointer:

auto paint(const ColorGetter f)
{
  return [=]() { return f(); };
}

while I am at it. Taking const& to arguments blindly is a bad habit. As is taking things by value blindly. Know what you are passing.