RVO: Return value passed by value even if explicitly assigned to a const reference

Question

I have a settings framework, which eventually caches values storing them into an std::map of boost::any.

Since I don't want the client to deal with exceptions, it provides a default value the settings framework would fallback in case the setting retrieval fails : this forces me to return the setting value by copy.

class SettingsMgr
{

    public:
        template<class T>
        T getSetting(const std::string& settingName, const T& settingDefValue)
        {
            try
            {
                if(cache.find(settingName) != cache.end)
                {
                     return any_cast<const T&>(cache.find(settingName)->second);
                }
                else
                {
                    cache[settingName] = someDbRetrievalFunction<T>(settingName);    
                    return any_cast<const T&>(cache.find(settingName)->second);
                }
             }
             catch(...)
             {
                 return settingDefValue;
             }
          }
        // This won't work in case the default value needs to be returned 
        // because it would be a reference to a value the client - and not the SettingsMgr - 
        // owns (which might be temporary etc etc)
        template<class T>
        const T& getSettingByRef(const std::string& settingName, const T& settingDefValue);

     private:
        std::map<std::string, boost::any> cache;
}

Now, I wasn't expecting this to be a big deal since I thought that thanks to RVO magic a reference to the cached value owned by the settings framework would have been retured - especially when the client explicitly encapsulates the return value in a const reference!

According to my tests it does not seem to be the case.

void main() {
    SettingsMgr sm;
    // Assuming everything goes fine, SNAME is cached
    const std::string& asettingvalue1 = sm.getSetting<std::string>("SNAME", "DEF_VALUE"); 

    // Assuming everything goes fine, cached version is returned (no DB lookup)
    const std::string& asettingvalue2 = sm.getSetting<std::string>("SNAME", "DEF_VALUE");

    ASSERT_TRUE(&asettingvalue1 == &asettingvalue2);  // Fails

    const std::string& awrongsettingname = sm.getSettingByRef<std::string>("WRONGSETTINGNAME", "DEF_VALUE");
    ASSERT_TRUE(awrongsettingname  == "DEF_VALUE"); // Fails, awrongsettingname is random memory
}

What are your functions *doing*? Please try to create a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve) and show us. Also, have you tried to step though the code in a debugger so you *know* which function is actually called? — Some programmer dude, Jun 20 '17 at 08:17
You are trying to help the compiler do its job. IIRC RVO will only kick in if you assign the result to a value. — StoryTeller - Unslander Monica, Jun 20 '17 at 08:19
`getSetting` returns by value, each value it returns is going to be a different object regardless of whether RVO eliminates the need for a second object inside the body of the function. — CB Bailey, Jun 20 '17 at 08:19
@Charles thanks for the comment. Makes sense. How can I workaround the current design to avoid the getSetting copy keeping the default value "feature" ? I would like to avoid forcing the client to use shared_ptr or any other disruptive solution. — codeJack, Jun 20 '17 at 08:26
To be honest, I don't understand why `getSettingByRef` wouldn't do what you want. — CB Bailey, Jun 20 '17 at 08:29
I will add the getSettingsByRef issue in my question, in the main function — codeJack, Jun 20 '17 at 08:36
`getSettingByRef` works perfectly in the first unittest, but fails whenever something goes wrong at retrieval time and the provided default value is to be returned. To me this seems pretty obvious because the default string is volatile and has not been stored anywhere. — codeJack, Jun 20 '17 at 08:41
That assertion failure looks to be because you have separate literals in main, unrelated to getSetting — Caleth, Jun 20 '17 at 08:45
How about providing method `T getSettingRef(const std::string& settingName, const T& settingDefValue)` and `T getSettingRef(const std::string& settingName, T&& settingDefValue) = delete;`, so you cannot use it with temporary. (User has still to guaranty lifetime of default value is longer than the returned one). — Jarod42, Jun 20 '17 at 09:30
Why is it important that the address of `asettingvalue1` and `asettingvalue2` are the same? — Chris Drew, Jun 20 '17 at 09:32
@ChrisDrew Because the user (in case of vector of settings etc) would not duplicate the item in memory. I came out with a possibly dirty but working solution : I will cache the default value with a negative time to live. THe default value will be stored in cache until the same parameter is retrieved again, of course if it fails again the value would be overwritten and so on. — codeJack, Jun 20 '17 at 09:45
@codeJack You mean this is a performance optimization? Are you sure this is not premature optimization? Have you measured the memory usage and determined that it is significant? — Chris Drew, Jun 20 '17 at 09:48

score 1 · Answer 1 · answered Jun 20 '17 at 09:57

1

You can go with the getSettingByRef version and prevent the possibility to pass any rvalue references:

    template<class T>
    const T & getSetting(const std::string& settingName, T&& settingDefValue) {
         static_assert(false, "No rvalue references allowed!");
    }

answered Jun 20 '17 at 09:57

x squared

3,173
1
26
41

putting it `= delete` as suggested by @Jarod42 would raise an error at compile time ? This problem should be identified at compile time, not at runtime. – codeJack Jun 20 '17 at 10:02
I chose static assert to give an explanatory message to the user. Before I accidentally had a throw there instead. – x squared Jun 20 '17 at 12:49

RVO: Return value passed by value even if explicitly assigned to a const reference

1 Answers1