Should compiler warn about unsafe behaviour when using temporary object on const ref type parameters?

Question

Let's consider following improper code:

#include <string>
#include <iostream>

struct C {
   C(const std::string &_s): s(_s) { }
   void run() {
      std::cout << "Here we are using the string " << s << "\n";
   }
   const std::string &s;
};

int main() {
   C a("/tmp/example.log");
   a.run();
   return 0;
}

Either g++ 6.3.0 as clang 3.8.1 compiles this code without warning, even with -W -Wall -Werror, but even for unwary programmer the behaviour simply says something is wrong here:

Here we are using the string Here we are usin

Why the results is not as one could naively expect? The lifetime of std::string object created from const char * is limited to the scope of constructor. So, when foo() constructor returns, the foo::s field has a reference to non existent object. Kaboom.

If someone understands the idea, it's easy to introduce the fix. It could be either explicit creation of std::string object and passing it as a parameter:

std::string s("/tmp/example.log");
C a(s);

The other one, which I consider a little bit safer is to change a foo::s field declaration to:

const std::string s;

making a local unmodifiable copy of the parameter, but it adds a cost of copying the object. Well, in this example copying of std::string doesn't automatically causes making a deep copy, but for own implemented class it could be completely different matter.

For C++11 there are few other methods available, as specified in this answer, but - for example - in embedded Linux environment when you are stuck to old compiler and some legacy code it's not necessarily a considerable option.

However, whatever solution would be chosen by programmer, one must know there is a problem. Even if I write my code properly, I dumbly assumed the compiler would be wise enough to detect such situations as obviously dangerous, if some other developer would fall into the trap of passing a const char * parameter. However, I decided to write some code to check the behaviour of compiler and I was disappointed - one can easily fall into the trap of having the reference to already non-existed object.

Passing a temporary object as a const reference is a recognized danger and it has been somehow addressed in C++11, but still passing the onus onto programmer to actively counteract such behaviour. So, is that a reasonable to expect a warning from the compiler, or rather one should rely on external static analysis tools?

Passing temporaries as ref-to-const parameters is not the problem here; storing the reference is. Introducing the warning you suggest would not be helpful. — You, Jun 15 '17 at 13:09
It's not really clear to me what your question is... you ask "Why the results is not as one could naively expect?" but then you appear to go on to answer your own question — M.M, Jun 15 '17 at 13:11
Rust has lifetime in their type, that would solve your issue. — Jarod42, Jun 15 '17 at 13:14
The question is basically: why doesn't the compiler give a warning for this situation — Xander Kyle Puckett, Jun 15 '17 at 13:14
I think it's because the compiler knows that passing a temporary object as a const-ref parameter is valid, but doesn't have a way to detect whether or not the parameter that's passed in is a temporary one, and so it assumes that storing it by reference as a member is no problem. — Xander Kyle Puckett, Jun 15 '17 at 13:18
@XanderKylePuckett C++ is flexible enough to create so many possibilities to shoot your own leg that it is impossible for a compiler to detect them all. C++ is a tool for professionals, unfortunately you cannot make a tool flexible and non restrictive for experienced users and safe for novices at the same time. At least I do not know any. — Slava, Jun 15 '17 at 13:27

Slava · Accepted Answer · 2017-06-15T13:20:16.333

3

Bind temporary to const reference works by design and should not issue any warnings. And it is not the source of problem in your code. Problem is that you use const reference as a member and not managing it's correctness. And even if you pass std::string as you suggested, that would not eliminate the problem - that string must outlive your object for your program to be correct. And creating member as std::string or const std::string is not little safer, but safe and proper way to do it in this situation. There are cases when you may want to store const or non const reference as a member, but you have to understand what you are doing and make sure that reference would not become dangled. Compiler unlikely to detect corner cases and issue you warning in this case.

edited Jun 15 '17 at 13:20

answered Jun 15 '17 at 13:12

Slava

43,454
1
47
90

1

There are legitimate reasons to store references as members though. It just needs to be made clear in the documentation that that is what the class is doing, and that clients need to be careful because of that. – Benjamin Lindley Jun 15 '17 at 13:16
@Slava: Lifetime of `std::string` - yes, this is what I had in mind, thank you for precising that. Member type: I agree - it's safe. But the proper part could be a subject of discussion. All of those approaches has some drawbacks in scope of either performance or safety (including code reusability). Thanks for the answers, I will change some parts of my code to safe way and put a more detailed documentation to other ones. – ArturFH Jun 15 '17 at 15:08
@ArturR.Czechowski I do not quite understand your concern - there is no such thing as free lunch. You want safety then you may pay by some inefficiency or memory usage. You want speed then you may have to be careful. And I am afraid you overestimate cost of keeping `std::string` as a member. In your case for example most probably there would be 0 cost, and your example with string in main and refernce in class could be even slower. – Slava Jun 15 '17 at 15:13

score 0 · Answer 2 · answered Jun 15 '17 at 13:15

The problem here IMO is not passing the temporary object as const ref, but storing it as such. Changing the member to const std::string s; (or even non-const) will resolve the issue.

If you plan to store it as reference, you must assure the object lifetime outside. Indeed, it is quite easy in C++ to shoot into your own leg if not careful enough.

And I really don't want the compiler to emit a warning when storing an input parameter as a reference, even though it might be dangerous if not careful, because there are valid use cases for that (i.e. all cases where the parameter lifetime exceeds the lifetime of the object and I don't want to copy it internally).

Note that the same applies to storing as a pointer etc.

Also, having the parameter passed by value (as opposed to const ref) does not solve the problem at all, because such a parameter copied as value is still a temporary, so when bound to a reference, it also goes away when the constructor is left.

Should compiler warn about unsafe behaviour when using temporary object on const ref type parameters?

2 Answers2