shared_pointer best practices

Question

A smart pointer is used in this small routine to spare the caller the hassle of free()ing the returned buffer (and protect against his failure to do so) :

char* toupper(const string& s){
  string ret(s.size(), char());
  for(unsigned int i = 0; i < s.size(); ++i)
    ret[i] = (s[i] <= 'z' && s[i] >= 'a') ? s[i]-('a'-'A') : s[i];
  return (char*) memcpy(
   chkedAlloc(1+ret.size(),*std::make_shared<char*>(nullptr)),
   ret.c_str(), 1+ret.size()
  );
}

chkedAlloc(n,A) returns new A[n] after catching std::bad_allocs.

This looks like a textbook use of smart pointers to make life easier & safer?

Do you see any fault with/possible improvement in this code?

improvements/comments:

• Return a string. It's destructor frees the associated storage when it goes out of scope.
• This is a case of "shared_ptr as a last resort". "shared" is more expensive than "unique".
• If you're thinking that :
  • a shared_ptr is thread-safe
  • a unique_ptr can't be passed to others

stop believing that. Owners can deal unique_ptrs, but one at a time can hold ownership.

• Consider the standard template algo std::transform.
• Don't use use std::string in your example code, prefer int, to avoid all kinds of side-discussions.

unique_ptr<char*> toupper(const string& s){
  string ret(s.size(), char());
  for(unsigned int i = 0; i < s.size(); ++i)
    ret[i] = (s[i] <= 'z' && s[i] >= 'a') ? s[i]-('a'-'A') : s[i];
  char *p = new char[1+ret.size()]; // try/catch(std::bad_alloc)
  return make_unique<char*>((char*) memcpy(p, ret.c_str(), 1+ret.size()));
}
void f() { string s = "hello"; .. 
  { char* p = *toupper(s); // this scope now owns the pointer
  } // leaving the scope: allocated memory on the heap freed by smart pointer
..
}

Answer 1

One major flaw in this code is here:

*std::make_shared<char*>(nullptr)

makes no sense whatsoever. It is just a convoluted and expensive way of obtaining a null pointer. The whole expression can be replaced with nullptr, so the smart pointer in this function is of no help to anyone.

It is in fact the newest iteration of the persistent error people keep making over and over and over and over again.

In C:

Foo foo = *(Foo*)malloc(sizeof(foo));

In C++:

Foo foo = *new Foo;

and now this:

Foo foo = *std::make_shared<Foo>();

All of the above are instances of the same bug. Don't do this. Dynamic allocation returns you a pointer so that you can store it somewhere. The pointer is not meant to be immediately dereferenced and forgotten.

Since the smart pointer in the function body, contrary to the expectations, is useless and doesn't save the caller any hassle whatsoever, the signature

char* toupper(const string& s){

is a bug. It should be

std::string toupper (const std::string& s) {

and the return statement should simply return ret. The whole

(char*) memcpy(
   chkedAlloc(1+ret.size(),*std::make_shared<char*>(nullptr)),
   ret.c_str(), 1+ret.size()
  );

isn't doing anything useful.

Finally, the loop

for(unsigned int i = 0; i < s.size(); ++i)
    ret[i] = (s[i] <= 'z' && s[i] >= 'a') ? s[i]-('a'-'A') : s[i];

can be rewritten as

std::transform(s.begin(), s.end(), ret.begin(), 
               static_cast<int (*)(int)>(::toupper));

(your own function name would conflict with just ::toupper so you need to specify the type)

Another option is

std::string toupper(std::string s){       // yes, yes, pass by value
   for (auto& c: s) c = std::toupper(c);
   return s;
}

Note ::toupper and std::toupper are two different functions.