4

I have a function returning a reference to an instance of my class "record".

record& get_record(int key) {
    return lookup(key);
}

That is effective it returns a reference and not by value. Now I modify it a bit.

record& get_record(int key) {
    if (valid(key))
        return lookup(key);
    else {
        record x;
        x.valid=false;
        return x; //Here I really want to return a temporary variable
                  // and not a reference to a local variable.      
    }
}

Is it correct that returning a reference to a local variable is not a good idea? and how do I return x in such a way that it becomes a temporary variable?

songyuanyao
  • 169,198
  • 16
  • 310
  • 405
user2304458
  • 363
  • 1
  • 2
  • 11
  • You declare the variable outside the function. You pass the reference to your funtion, and then the function modifies it. Also this is probably a dupe. – babu646 May 25 '18 at 07:55
  • Can't you just return a pointer and make the second case return the null pointer? – Petr Skocik May 25 '18 at 07:55
  • Possible duplicate of [C++ Returning reference to local variable](https://stackoverflow.com/questions/4643713/c-returning-reference-to-local-variable) – babu646 May 25 '18 at 07:56
  • 5
    You may not return references to local variables. The local variable is destroyed when leaving scope and the reference gets dangling. Once in the past, I had a similar case where I wanted to return references. When the resp. method fails I have to return a reference to a dummy. I made a `static` dummy instance for this. – Scheff's Cat May 25 '18 at 07:56
  • Not a duplicate (of that question) - this is about good design; the linked question is about the technicalities. – MSalters May 25 '18 at 07:57
  • Guess you try to make things more complex than they can be. Why you need reference for return value? Why not pointer. – i486 May 25 '18 at 08:02
  • Aside: the idea of `lookup` not being able to handle the key not existing means that you likely need to do 2 look ups, once to see if the key exists and the 2nd to get the value. If you change `lookup` to return an iterator (or nullable smart pointer) then you can half the work done in this function! – UKMonkey May 25 '18 at 09:32
  • May I ask what you are actually trying to accomplish? (It looks like you want to avoid a copy if the key is found.) You will not get a more elegant solution than returning by value, however, so this poses the question whether the potential performance gain outweighs a less elegant call syntax. – Arne Vogel May 25 '18 at 11:12
  • @Arne Vogel My question is, of course, a simplification of what I really try to do. I have two databases "new_db" and an "old_db". The "new_db" will be created from keys referring to parts of the "old_db", which will be copied, and if no part is found for a given key it will be constructed and added to "new_db". I thought it would be smart that "get_record" not only was responsible for finding a record but also for creating the one it couldn't find based on key value and some other parameters. However, I now realize that I maybe should choose another design. – user2304458 May 25 '18 at 12:27

5 Answers5

7

Is it correct that returning a reference to a local variable is not a good idea?

Yes. The local object will be destroyed when get out of the function so the returned reference is always dangled.

You might make x a static variable.

record& get_record(int key) {
    if (valid(key))
        return lookup(key);
    else {
        static record x;
        x.valid=false;
        return x;
    }
}

Note that the returned reference will always refer to the same object, i.e. x.

songyuanyao
  • 169,198
  • 16
  • 310
  • 405
  • 4
    This of course is asking fro problems if you call it twice i.e. `Foo(get_record(1), get_record(2))` will create two references to the same `x`. – MSalters May 25 '18 at 07:58
  • @MSalters Yeah, this concept fits much better if a `const` reference is returned. At least, the `valid` member could be made `const` to prevent accidental access a bit better. – Scheff's Cat May 25 '18 at 07:59
  • `x` is not a temporary as asked by OP. – YSC May 25 '18 at 08:09
  • @YSC I don't think OP wants a temporary (as defined as the C++ term), I suppose he just wants return something for the false case, which could be used outside for determining. – songyuanyao May 25 '18 at 08:13
  • 5
    Please don't write code that compiles but has horrible hidden consequences without at least pointing out these consequences in big, bold warning messages. "will always refer to the same object" just doesn't have the same impact as "you will get spooky action at a distance if you call the function twice" or "this will get you data races and turn your entire program into undefined behavior". – Sebastian Redl May 25 '18 at 08:14
  • 2
    @SebastianRedl I can understand data races, but what do you mean " spooky action" when call the functiioin twice? I know it has potential issues but it depends on OP's scenario; without details we can't give more special and complete warning messages. – songyuanyao May 25 '18 at 08:20
  • I think the ideal would be a temporary variable, but what about an additional function which creates a temporary variable. record make_temporary(record &a) { return a; } Instead of just returning x I will do return make_temporay(x); – user2304458 May 25 '18 at 08:22
  • @user2304458 You can't do that. The temporary will be destroyed immediately. And we can't bind temporary to non-const reference. – songyuanyao May 25 '18 at 08:30
  • @usersongyuanyao A function either returns a pointer or a reference or by value. If it returns by value a temporary variable is created, if I understand it correctly, so my function make_temporary will return a temporary variable. – user2304458 May 25 '18 at 08:33
  • 1
    @user2304458 Yes it does return a temporary. But you can't bind it to non-const reference, so you can't return it in a function which returns non-const reference. [Sample](https://wandbox.org/permlink/kCxSXGr5RloxHxTG) – songyuanyao May 25 '18 at 08:34
  • @usersongyuanyao Thanks that you point that out, but actually I am going to use a const reference :) – user2304458 May 25 '18 at 08:37
  • @user2304458 Note that the temporary will be destroyed immediately, so you can make it `const` reference but the returned reference is still a dangled reference. – songyuanyao May 25 '18 at 08:40
7

This is worse than a bad idea, it is undefined behavior and result in most of the cases to a crash. This is bad (TM).

What you could do is changing the return type of get_record so it returns a smart pointer. If key is valid, it returns an observer pointer to it. Otherwise, it returns an owning smart pointer to a freshly created object:

#include <memory>
#include <iostream>

struct record { int n; } some_record{42};

std::shared_ptr<record> get_record(bool b)
{
    if (b == true) {
        return std::shared_ptr<record>{&some_record, [](record*){}}; // see explanation ^1
    }
    return std::shared_ptr<record>{new record{0}};
}

int main()
{
    std::cout << get_record(true)->n << "\n";  // this is some_record
    std::cout << get_record(false)->n << "\n"; // this is a temporary
}

1) About [](record*){}: this no-op lambda given as the second argument to std::shared_ptr::shared_ptr() is invoked when the smart pointer is destroyed. It replaces the default deleter of std::shared_ptr whose behavior is to call delete on the owned pointer.

About why your design is flawed. In fact, making get_record return a reference makes it not consistent. What you want is:

  • if key is valid return a reference to an existing/permanant object, and
  • return a temporary object otherwise.

Those two are mutually exclusive, and your function doesn't make sense: what does get_record return semantically?

YSC
  • 38,212
  • 9
  • 96
  • 149
  • You can indeed return a `shared_ptr`. You don't need to make a distinction between observer/owning. The distinction would be that for invalid keys, the returned `shared_ptr` is the exclusive owner. – MSalters May 25 '18 at 08:01
  • @MSalters yes obviously ^^ see update (it's still early here) – YSC May 25 '18 at 08:05
  • a little pedantic - but returning a reference to a temporary object isn't undefined behaviour, but it's the using it afterwards that is. – UKMonkey May 25 '18 at 09:16
  • @UKMonkey That is right, but I don't want to go down to such details in this answer. I could kink to it though, will do, thx. – YSC May 25 '18 at 09:24
  • @YSC Could you explain the lambda usage in the `b == true` case?, I think I like what is going on but I just don't quite get it... I think its meant to be a parameter of `&some_record?` - I also think I am way off :o – code_fodder May 25 '18 at 09:36
  • @YSC ahhh... cool thanks ... I guess you don't want shared_ptr to delete `some_record` (not sure what would happen in that case! ... clever stuff : ) – code_fodder May 25 '18 at 09:59
1

If you are allowed to modify the get_record function you can change the return type to pointer to record instead of reference to record.

record* get_record( int key )
{
    if( valid( key ) ) return &lookup( key ); 
    else               return nullptr;
 }

However, this approach needs two guarantees:

  • the lookup function must return a reference to record
  • the record returned by lookup must still live when lookup returns (e.g. record is an element of some sort of container and lookup returns its reference)
Guido Niewerth
  • 156
  • 7
0

As others already detail why returning a reference to a local is bad, ill just provide an alternative: Exceptions. Though you could write a custom exception, arguably an std::out_of_range exception would seem in place (as the key lies outside of the range of valid keys, which is exactly what std::map::at does).

Have a look at: How to throw a C++ exception

record& get_record(int key)
{
  if (valid(key))
  {
    return lookup(key);
  } else {
    throw std::out_of_range("The provided key is invalid");
  }
}

You will now, obviously, have to catch the exception in the code calling get_record, otherwise your program will still terminate.

Lanting
  • 3,060
  • 12
  • 28
0

Returning a reference in itself doesn't produce undefined behaviour, but if you attempt to modify it, then you will.

Accessing an object outside of its lifetime is undefined behavior.

int* foo(void) {
    int a = 17; // a has automatic storage duration
    return &a;
}  // lifetime of a ends
int main(void) {
    int* p = foo(); // p points to an object past lifetime ("dangling pointer")
    int n = *p; // undefined behavior
}

http://en.cppreference.com/w/c/language/lifetime

If you have access to C++17, you could implement it using std::optional. Note the use of std::reference_wrapper, because use of a reference in std::optional makes your program ill-formed.

std::optional<std::reference_wrapper<record>> get_record(int key) {
    if (valid(key))
        return std::optional<std::reference_wrapper<record>>(lookup(key));
    else
        return std::nullopt;
}

Without C++17, you could just return a pointer to your record:

record* get_record(int key) {
    if (valid(key))
        return &lookup(key);
    else
        return nullptr;
}

Or if you prefer, you can keep the reference return type, and throw an exception to indicate a missing record. Though this is my least preferred approach as it makes it easy to call get_record without wrapping in a try / catch.

record& get_record(int key) {
    if (valid(key))
        return &lookup(key);
    else
        throw std::out_of_range("Invalid record key!");
}
Mark Ingram
  • 71,849
  • 51
  • 176
  • 230