1

I have inherited a pile of C++ code, saturated with std::shared_ptrs, most [all?] of which are unnecessary and are, I suspect, degrading performance. It is not feasible to change everything in one go, so I'm changing the code in tranches and doing performance tests.

An issue I'm running into is the interface in the method hierarchy between my new "raw" pointer code, and the underlying shared_ptr stuff. A contrived example (I know it could be simplified...):

SomeObject *MyClass::GetSomeObject(const std::string& aString)
{
  //for the underlying shared pointer methods
  std::shared_ptr<std::string> tmpString = make_shared<std::string>(aString);

  //call the method using my local shared pointer
  std::shared_ptr<SomeObject> someObj = GetTheObject(tmpString);

  //The line below gives compiler warning: "The pointer points to memory allocated on the stack" 
  return someObj.get(); // a pointer to an object in std::map
}

I know that GetTheObject() is returning a pointer to an object in a persistent std::map, so that memory will be in good standing after we exit GetSomeObject() and the shared pointer [and its wrapped raw pointer] have gone out of scope.

I'm not in the habit of ignoring warnings, SO:

Questions:

  1. Is the warning because the compiler is worried about the scope of the shared pointer rather than the object pointed to? [i.e. could I ignore it in this instance?]

  2. If it is a real problem, are there any neat ways around this (that do not involve building wrapper classes and such workarounds...) ?

Stephen Baldwin
  • 31
  • 5
  • 5
    `someObj` would try to delete the object, it's a real problem. – apple apple Dec 22 '21 at 17:28
  • 1
    I'm confused what performance improvement you believe you will get from changing the use of `shared_ptr` to raw pointers. This snippet at least will have no gain, since it still first copies a `shared_ptr` causing the ref count to be increased (and decreased when returning) – UnholySheep Dec 22 '21 at 17:39
  • Hello @apple apple: On further investigations [breakpoints, watch windows], `return someObj.get();` is returning what I suspected/hoped: the address of the object as stored in, and retrieved from, the persistent `std::map` accessed in the `GetTheObject(...)` call. And this address is being successfully passed to the calling method. Clearly, the retrieved map object is not itself the target of deletion. So unless there are other issues to be considered, 'GetSomeObject(...)` is returning a perfectly valid memory address...or so it would appear to me??? – Stephen Baldwin Dec 22 '21 at 19:21
  • 1
    You get a pointer stored in `someObj`, then `someObj` immediately goes out of scope. What does the shared_ptr do when it is destroyed? – BoP Dec 22 '21 at 20:56
  • Hello @BoP: Well the same argument would apply to any locally-defined construct that returns a pointer would it not? The issue at hand is whether the target memory itself goes out of scope. In my case, it does not. I don't know the internals - but `malloc` allocates memory and returns a pointer to that memory. `malloc` is not a system call so I presume it uses local variables that go out of scope - but nonetheless it returns a valid pointer to a valid chunk of memory that is assigned to a pointer in user code, and can be passed up a calling chain and still be entirely valid. Thoughts? – Stephen Baldwin Dec 23 '21 at 01:24
  • @Stephen - I think that we, the commenters, are concerned that `someObj` might be the last shared_ptr holding a specific address, and thus will create a dangling pointer during the return. – BoP Dec 23 '21 at 09:01

2 Answers2

2

If I understand you correctly, you're replacing smart pointers with dumb pointers, in 2021, and you're now facing the exact problem that smart pointers intended to solve.

The warning is 100% accurate, and I'm pleasantly surprised the compiler looked deep enough.

The solution is simple: return a shared_ptr<SomeObject>. If you want efficiency improvements, there are two real improvements possible. C++11 introduced move constructors, and moving shared_ptr is faster than copying. The compiler will use the move ctor for return someObj; since someObj goes out of scope

Secondly, shared_ptr is a heavy-weight alternative to unique_ptr. At times, you may be able to downgrade to the latter.

MSalters
  • 173,980
  • 10
  • 155
  • 350
  • Hello @MSalters: Thanks for the reply. Could you expound on the problems shared pointers were supposed to solve? My [admittedly limited] understanding was that they solve the problem of garbage collection: `new`, `delete`, and all that. I've used static variables and a counter to achieve the same result... My issue is sub-performing code that has to run faster to be of use. Shared pointers are part of the investigation and may or may not be relevant. Whether or not it is ok to use raw pointers in 2021 doesn't bother me much! Thanks for the info on unique pointers. I shall check it out. – Stephen Baldwin Dec 23 '21 at 01:37
  • Forgot: one more thing: see reply to @"apple apple". The code actually works in that a valid pointer to valid memory is passed back to the function calling `GetSomeObject()` ... – Stephen Baldwin Dec 23 '21 at 01:39
  • Well, the pointer is **not** valid, "apple apple" is right; the memory has been released to the free store (but likely not all the way back to the OS). It hasn't crashed **yet**. Smart pointers, but also classes like `vcetor` and `string` allow you to treat variable-sized allocations as if they were plain values like `int`. It's not Garbage Collection like Java or .Net. `someObj` still goes out of scope in your code, and there's no GC that takes over responsibility for that memory. – MSalters Dec 23 '21 at 11:07
0

I have similar code in my project. I agree that the proper solution is probably just to commit fully to the smart pointers and use them properly. However, I don't want to churn through piles of perfectly functional code, but I also want the warnings to go away. I was able to work around the warning with something like:

SomeObject *MyClass::GetSomeObject(const std::string& aString)
{
  //for the underlying shared pointer methods
  std::shared_ptr<std::string> tmpString = make_shared<std::string>(aString);

  //call the method using my local shared pointer
  std::shared_ptr<SomeObject> someObj = GetTheObject(tmpString);

  SomeObject *pRet = someObj.get();
  return pRet; // a pointer to an object in std::map
}

I'm a little worried that at some point the compiler will get smarter and detect that as a warning as well, but it seems OK for now. (Visual Studio 2022 v17.1) Hope that helps!

gdunbar
  • 571
  • 4
  • 11