Is there a way to consistently reference-capture a returned variable and use it in a destructor when-CodePudding

I've been working on adding transparent result-caching functionality to some computationally intensive code with multiple return statements. Whenever the function returns for any reason I want to take the last known value of the working buffer (val in the code below) and put it into a cache so I don't need to calculate it again in the future. Here's a basic reproduction of what I'm attempting:

#include <iostream>
#include <ranges>
#include <algorithm>
#include <map>
#include <string>
#include <functional>

struct ScopeExit
{
    ScopeExit(std::function<void()> f) : m_f(f) {}
    ~ScopeExit() {m_f();}

private:
    std::function<void()> m_f;
};

std::map<int, std::string> g_cache;

std::string func(const int key)
{
    std::string val;

    if (g_cache.contains(key))
    {
        // returning from the cache like this does NOT work
        return g_cache.at(key);

        // assigning the cached value to 'val' prior to returning DOES work
        //val = g_cache.at(key);
        //return val;
    }

    val = "hello world";

    // add the result to the cache just before we give the result to the caller
    ScopeExit scopeExit1([key, &val](){
        auto [iter, inserted] = g_cache.try_emplace(key, val);
        if (inserted)
            std::cout << "inserted \"" << iter->second << "\" into cache for key = " << iter->first << std::endl;
    });

    // normally a bunch of math happens here but I'm using a string for this example

    std::ranges::fill(val, 'b');

    if (2 == key)
        return val;
    
    std::ranges::for_each(val, [](auto& v){v  = 1;});

    return val;
}

int main()
{
    auto a = func(1);
    auto b = func(2);

    // these should pull from cache
    auto c = func(1);
    auto d = func(2);

    std::cout << "key = 1: " << a << std::endl;
    std::cout << "key = 2: " << b << std::endl;
    std::cout << "key = 1 (cached): " << c << std::endl;
    std::cout << "key = 2 (cached): " << d << std::endl;

    return 0;
}

https://godbolt.org/z/Ma314evhb

I noticed that when it was returning from the cache directly (e.g. return g_cache.at(key);), all cached values were empty, as if they had been moved from prior to the scopeExit1 destructor code running.

inserted "" into cache for key = 1
inserted "" into cache for key = 2
key = 1: ccccccccccc
key = 2: bbbbbbbbbbb
key = 1 (cached): 
key = 2 (cached):

Further research led me to the "automatic move from local variables and parameters" section of https://en.cppreference.com/w/cpp/language/return, which I believe states (paraphrased - please correct me if I'm wrong):

if expression is a non-volatile object type declared in the body, it will treat the return value as an rvalue expression and pick the move constructor if available, otherwise it will treat expression as an lvalue and pick the copy constructor.

I wrote an instrumented version of the same thing here: https://godbolt.org/z/6xrdTddsM, which confirms that the move constructor is being called on return and that it's being accessed afterwards.

(0x7ffe1bf1f330) default constructor
(0x7ffe1bf1f410) move constructor (source = 0x7ffe1bf1f330)
(0x210dee8) copy constructor (source = 0x7ffe1bf1f330)
inserted "" into cache for key = 1
(0x7ffe1bf1f330) destructor

(0x7ffe1bf1f330) default constructor
(0x7ffe1bf1f3f0) move constructor (source = 0x7ffe1bf1f330)
(0x210df38) copy constructor (source = 0x7ffe1bf1f330)
inserted "" into cache for key = 2
(0x7ffe1bf1f330) destructor

(0x7ffe1bf1f330) default constructor
(0x7ffe1bf1f3d0) copy constructor (source = 0x210dee8)
(0x7ffe1bf1f330) destructor

(0x7ffe1bf1f330) default constructor
(0x7ffe1bf1f3b0) copy constructor (source = 0x210df38)
(0x7ffe1bf1f330) destructor

key = 1: ccccccccccc
key = 2: bbbbbbbbbbb
key = 1 (cached): 
key = 2 (cached): 
(0x7ffe1bf1f3b0) destructor
(0x7ffe1bf1f3d0) destructor
(0x7ffe1bf1f3f0) destructor
(0x7ffe1bf1f410) destructor
(0x210df38) destructor
(0x210dee8) destructor

Interestingly, if I replace the return g_cache.at(key); with val = g_cache.at(key); return val; it elides the move and constructs the string in-place in the caller's stack and works the way I want. However, because I don't think this elision is mandatory (I'm not returning prvalues) I think it only coincidentally works, although the undefined behavior sanitizer doesn't appear to mind.

Side questions: Is this actually consistent / guaranteed / well defined behavior? And if so, is there any way to make it less fragile from a maintainability point of view? (e.g. I added a different return variable and oops I unknowingly completely broke caching)

(0x7ffee22bc150) default constructor
(0x1c1dee8) copy constructor (source = 0x7ffee22bc150)
inserted "ccccccccccc" into cache for key = 1

(0x7ffee22bc130) default constructor
(0x1c1df38) copy constructor (source = 0x7ffee22bc130)
inserted "bbbbbbbbbbb" into cache for key = 2

(0x7ffee22bc110) default constructor
(0x7ffee22bc110) copy assign (source = 0x1c1dee8)

(0x7ffee22bc0f0) default constructor
(0x7ffee22bc0f0) copy assign (source = 0x1c1df38)

key = 1: ccccccccccc
key = 2: bbbbbbbbbbb
key = 1 (cached): ccccccccccc
key = 2 (cached): bbbbbbbbbbb
(0x7ffee22bc0f0) destructor
(0x7ffee22bc110) destructor
(0x7ffee22bc130) destructor
(0x7ffee22bc150) destructor
(0x1c1df38) destructor
(0x1c1dee8) destructor

I also noticed that it works the way I want if I comment out the move constructor and move assignment operators - this lets it fall back to the copy constructor according to the return rules quoted above. This approach seems to be a consistent and correct way to achieve what I want, but isn't very practical in my actual application where I'm working with std::vector<float>.

Is there any way to ensure that my returned variable will not be moved from so that I can use it in my scope exit destructor?

CodePudding user response：

I assume since you're caching the values that performance matters to you, so constantly copying std::string's is probably not what you want to do in this case.

For example, this line here: return g_cache.at(key); will always copy construct the std::string (assuming the return type of the function is by value).

I also find the use of the ScopeExit to add unnecessary complexity to your situation and will force you to capture the std::string by value or explicitly copy construct a new std::string when returning.

Also, you ideally don't want a global cache as that can get you in to trouble with multi threaded code.

That being said, in keeping with your current design (having a global cache and caching the value in the func function) here is a more efficient way you can accomplish what you're after.

static std::unordered_map<int, std::string> cache{ };

static const std::string& get_value( int key ) 
{
    if ( auto it{ cache.find( key ) }; it != std::end( cache ) ) 
    {
        return it->second;
    }
    
    std::string value{ "Some complex computed value" };
    const auto[ it, _ ]{ cache.insert( { key, std::move( value ) } ) };
    return it->second;
}

CodePudding user response：

Is there any way to ensure that my returned variable will not be moved from so that I can use it in my scope exit destructor?

Yes: make a copy of it when you try to return it. Turn all of your return a;s into return std::string(a); or whatever type you're using.

A much easier solution is to properly separate your concerns. You need a function which computes a value, and you need a function that deals with the cache. Your problem comes from trying to do both in the same function.

So... don't do that:

std::string uncached_func(const int key)
{
    std::string val = "hello world";

    // add the result to the cache just before we give the result to the caller
    ScopeExit scopeExit1([key, &val](){
        auto [iter, inserted] = g_cache.try_emplace(key, val);
        if (inserted)
            std::cout << "inserted \"" << iter->second << "\" into cache for key = " << iter->first << std::endl;
    });

    // normally a bunch of math happens here but I'm using a string for this example

    std::ranges::fill(val, 'b');

    if (2 == key)
        return val;
    
    std::ranges::for_each(val, [](auto& v){v  = 1;});

    return val;
}

std::string cached_func(const int key)
{
    //If you use `contains` followed by `at`, you're doing it wrong.
    if (auto it = g_cache.find(key); it != g_cache.end())
    {
        return it->second;
    }

    auto ret = uncached_func(key);
    //No point in `try_emplace`, because it obviously wasn't cached if we're here.
    g_cache.emplace(key, ret); 
    return ret;
}

Indeed, you can move cached_func into a generic type that also stores its own cache. This allows you to avoid relying on a global variable which will bite you when someone eventually tries to thread this code:

template<typename> class CachedAccess;

template<typename Ret, typename Arg>
class CachedAccess<Ret(Arg)>
{
private:
  std::map<Arg, Ret> cache_;
  std::function<Ret(Arg)> callable_;

public:
  explicit CachedAccess(std::function<Ret(Arg)> callable) : callable_(callable)
  {}

  Ret operator()(Arg arg)
  {
    if (auto it = g_cache.find(arg); it != g_cache.end())
    {
        // returning from the cache like this does NOT work
        return g_cache.at(arg);
    }

    auto ret = callable_(arg);
    g_cache.emplace(arg, ret); 
    return ret;
  }
};