0

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 .. }
Improve this question
16
  • 7
    This is an even better case to return a std::string. Commented Sep 16, 2021 at 20:02
  • 1
    What is chkedAlloc? That return statement is undecipherable to me. And I see a lot of code in a day. I can't think of any way *std::make_shared<char*>(nullptr) can do anything helpful. Commented Sep 16, 2021 at 20:07
  • 1
    The use of make_shared() in this code makes no sense. It is allocating an owned char* pointer (not a char[] buffer!) that doesn't point anywhere. memcpy() needs allocated memory to copy to. What does chkedAlloc() actually do? This looks like very dangerous code to me. Especially since you mention free(), which shared_ptr does not use, but does chkedAlloc() really require it? Commented Sep 16, 2021 at 20:08
  • 6
    "shared_pointer best practices" simple: avoid using them at all unless absolutely necessary. Designs with clear-cut ownership relations are simpler and cleaner in general. Also, while std::unique_ptr is perfectly fine, std::shared_ptr has some unexpected overhead involved (due to its thread safety obligations) Commented Sep 16, 2021 at 20:08
  • 3
    user4581301: just tested with unique_ptr. You're absolutely right. In my mind, unique somehow meant "won't relinquish"! While unique actually means 1 owner (at a time), but ownership can be passed around. Commented Sep 16, 2021 at 20:39

1 Answer 1

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

If the function allocates a int* p=new int[35], makes a q=unique_ptr<int*>(p) and returns p, what happens to the allocated memory when the caller is done with the returned int* ?
This is an entirely different question. If you want to ask a question, please press the "Ask Question" button.
Side note:: toupper (and the rest of its ctype friends) takes an int for it's argument rather than a char so that it can better handle out-of-band signals like EOF. This has the unfortunate side effect of sign-extending any signed characters provided, potentially changing their value and causing chaos. Doesn't happen all that often, but if you get weird results, cast c to unsigned char (std::toupper(static_cast<unsigned char>(c));) just to be sure.
@WasfiJAOUAD aside from that, q=unique_ptr<int*>(p) is a type error. You need a std::unique_ptr<int[]> to correctly hold p, but what you actually want is std::vector<int> p(35);

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.