1

Consider this example:

#include <iostream>

struct Thing
{
    Thing(int const& ref) : ref(ref) {}
    int const& ref;
};

int main()
{
    int   option_i = 1;
    short option_s = 2;

    Thing thing_i(option_i);
    Thing thing_s(option_s);

    std::cout << thing_i.ref << "\n";   // 1
    std::cout << thing_s.ref << "\n";   // 2

    option_i = 10;
    option_s = 20;

    std::cout << thing_i.ref << "\n";   // 10
    std::cout << thing_s.ref << "\n";   // 2 <<< !!!
}

Now, I understand WHY this is the case: The short option_s is converted to an int using a temporary object. The reference to that temporary is then passed on to the constructor Thing::Thing, i.e. equivalent to

// Thing thing_s(option_s);
int __temporary = option_s;
Thing thing_s(__temporary);

However, this may in many cases not be desired, as it is very hard to spot the difference. In this case the temporary is at least still alive, but this could easily be a dangling reference-to-temporary as well.

Do you know of any way (design pattern, gcc compiler option, static analysis tool or other) to detect such a const-reference-to-temporary case?

Would you sacrifice const-correctness in order to detect such cases?

Community
  • 1
  • 1
mbschenkel
  • 1,865
  • 1
  • 18
  • 40

2 Answers2

3

Just write another constructor and mark it =delete as:

template<typename T>
Thing(T const &) = delete;

Now if you pass any argument other than int, you'll get compilation error.

Thing thing_s(option_s); //error

If you want only short (or set of known types) to be deleted, then you can use this:

Thing(short const &) = delete;
Thing(long const &) = delete; //lets disable long as well.

Hope that gives you the basic idea.

Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • Your option is probably best, but you can also eliminate the constructor via SFINAE. You'd template the constructor and use `enable_if` to ensure it's an `int`. – Edward Strange Dec 08 '16 at 17:24
  • 1
    @CrazyEddie: there are more than one ways to do it. I find this approach cleanest. It also tells you the constructor/function you're trying to invoke has been deleted, thereby making you realize the "disallowed" types. And the compiler *stops* searching for more overloads, that makes it **fail-fast** approach, which is good. – Nawaz Dec 08 '16 at 17:28
  • 1
    @CrazyEddie: Explicitly deleting gives you a nice error message, whereas the absence of a constructor may be quite puzzling. – Kerrek SB Dec 08 '16 at 17:30
  • 1
    @KerrekSB: Hmm. Yes. If you dont want that, then maybe, `Thing(int &&)=delete;` should be added to the class, as well. – Nawaz Dec 09 '16 at 02:31
1

A good way to define constructors for classes that bind references is to take the argument by address:

struct Foo
{
   Bar const & ref;
   Foo(Bar const * b) : ref(*b) {}
};

Usage:

Bar x;
Foo y(&x);      // OK
Foo z(&Bar{});  // error: cannot take address of prvalue

This requires the user to go out of their way to obtain the address of a prvalue, and thus communicates clearly that you expect the address of an object that lives already, and presumably continues to live past your instance's lifetime.

Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
  • It will however also force me to check the validity of the pointer in the constructor. – mbschenkel Dec 08 '16 at 17:12
  • @mbschenkel: You cannot check the validity of pointers in general, so I wouldn't bother. You could perhaps document the precondition ("`b` must be dereferenceable"). – Kerrek SB Dec 08 '16 at 17:29