I've simple class method like
Task TaskSample::Create(void)
{
Task task;
return task;
}
and got warning taking address of temporary. Is something wrong with this code ? I prefer to not use pointer here
views:
197answers:
4
+12
A:
If that is actually what your code is, then the compiler is probably in error.
More likely, however, you actually wrote this:
Task& TaskSample::Create(void)
{
Task task;
return task;
}
Remove the & to return by value, instead of by reference. Returning by reference there makes no sense because task will be destroyed when the function returns.
Billy ONeal
2010-07-27 00:14:56
+1
A:
Modern compilers can optimize return value to avoid copying overhead. Often returning by value doesn't hurt performance at all.
But if you need to return by reference, use shared_ptr instead.
shared_ptr<Task> TaskSample::Create(void)
{
shared_ptr<Task> ptr(new Task(...));
return ptr;
}
If you chose to return dynamically allocated memory, `auto_ptr` [or possibly `unique_ptr` if you have access to C++0x] is probably more appropriate here.
Dennis Zickefoose
2010-07-27 02:15:09
auto_ptr has move semantics and easy to misuse. The default general purpose smart pointer should be shared_ptr, because it behaves like a plain pointer plus it does memory delocation for you. The other smart pointers are useful only in special cases and should not be considered as replacements for plain pointers.
2010-07-27 02:32:25
@m141: Actually, unique_ptr is the better choice if it is available. However, it's available on C++0x only. You get most of the benefits of shared_ptr, but you can define ownership of the object much more simply.
Billy ONeal
2010-07-27 02:37:35
@rn141: move semantics are what are required here. All smart pointers behave like a pointer. All smart pointers do memory deallocation. Plain pointers should be the absolute last choice as they are not exception safe (and most likely to be used incorrectly), and only when ownership is clearly defined to belong somewhere else. This is the perfect case for auto_ptr (or in the newer version of c++ unique_ptr
Martin York
2010-07-27 03:12:25
We use pointers in order to avoid copying heavy objects or for shared ownership. If ownership is limited to a particular scope, then scoped_ptr could be more appropriate (please leave unique_ptr alone until it becomes a standard) When it works, elide optimization is much better than any smart pointer. shared_ptr can work where auto_ptr can't. There are plenty of online blogs and articles about auto_ptr screw ups. Herb Sutter published several GotWs about auto_ptr troubles. These days I don't any auto_ptr in industrial C++ code. 99% of smart pointers are shared_ptr and the rest is scoped_ptr.
2010-07-27 03:56:45
`scoped_ptr` is not an option in this case; it can not be returned from a function. `shared_ptr` will often provide the wrong semantics; you are giving up ownership of the object. Obviously, returning a raw pointer is sub-optimal. If you aren't willing to consider C++0x features [a perfectly reasonable limitation], then that just leaves `auto_ptr`. The problems with `auto_ptr` only arise when you expect it to provide semantics it doesn't, and in this case you want the semantics it offers.
Dennis Zickefoose
2010-07-27 04:20:05
Quote:"then that just leaves auto_ptr". That is a strong statement, don't you think? Every quality C++ code I see these days doesn't use auto_ptr. 99% of time it is shared_ptr. Raw pointers are used more often than auto_ptr. Competent programmers are well aware of smart pointers' cost and know how to handle raw pointers. The ultimate owner of any object is the coder. He can emulate any restrictions on the ownership by using it in one way or another. But he can't do whole lot things with auto_ptr, which he can do with shared_ptr. auto_ptr is a minefield, even for experienced programmers.
2010-07-27 05:25:19
You know one thing you *can* do with `auto_ptr` that you *can't* do [reliably] with `shared_ptr`? Stuff it into a `scoped_ptr`. You don't have to emulate restrictions, you can let your code speak for itself. Using shared resources makes identifying when resources will be freed considerably more difficult. By using `shared_ptr` unnecessarily, you are replacing one "minefield" with another.
Dennis Zickefoose
2010-07-27 06:00:04
Drifting away from the original question? shared_ptr is a natural default solution. auto_ptr and scoped_ptr are a niche tools. Both can be replaced by shared_ptr, but the opposite isn't true. That is why shared_ptr is used 100 times more often in production code than auto_ptr. auto_ptr is too inflexible and a maintenance nightmare. Using excessive shared resources is a design related problem, not shared_ptr's problem. It takes very little to get burned with auto_ptr. You can safely transfer ownership from shared_ptr to scoped_ptr if use count is 1 (why would you want to do that if it is >1?)
2010-07-27 06:53:59
Btw, have you ever wandered why auto_ptr is being deprecated and unique_ptr should take it place? Or, why boost doesn't have unique_ptr? It is because auto_ptr is a minefield and standard needs to do something about. Boost has scoped_ptr and need not to bother fixing auto_ptr.
2010-07-27 21:42:52
A:
The best would be, to pass the object by reference as follows
void TaskSample::Create(Task& data)
{
....
}
-1: This may have been true at one time, but now you're better off doing the ideomatic thing and just returning the object. The compiler's going to optimize the copy if necessary, and the result is better code.
Billy ONeal
2010-07-27 02:08:40
+2
A:
Both of the following code snippets produce this error in MS C++:
warning C4172: returning address of local variable or temporary
Task* Create(void)
{
Task task;
return &task;
}
Task& Create2(void)
{
Task task;
return task;
}
MSDN documentation describes the warning quite succintly:
Local variables and temporary objects are destroyed when a function returns, so the address returned is not valid.
In order to return a pointer to an object you need to call operator new as an object allocated on the heap will not go out of scope:
Task* Create(void)
{
Task* task = new Task();
return task;
}
Don't forget to delete that task once you are done with it:
Task* task = Create();
delete task;
Alternatively you can use a smart pointer:
void Test () {
boost::scoped_ptr<Task> spTask = Create();
spTask->Schedule();
} //<--- spTask is deleted here
I would instead rely on RVO and actually use the code that you posted and which is most likely not the code giving you a warning.
void Test() {
Task task = Create();
}
Task Create(void)
{
Task task;
task.start = 10;
return task;
}
This may generate something equivalent to this, so really, there is no copy constructor overhead.
void Test() {
Task task;
Create(&task);
}
Task* Create(Task* __compilerGeneratedParam)
{
__compilerGeneratedParam->start = 10;
return __compilerGeneratedParam;
}
Igor Zevaka
2010-07-27 02:11:42