Avoiding memory leaks while mutating c-strings

Question

For educational purposes, I am using cstrings in some test programs. I would like to shorten strings with a placeholder such as "...".

That is, "Quite a long string" will become "Quite a lo..." if my maximum length is set to 13. Further, I do not want to destroy the original string - the shortened string therefore has to be a copy.

The (static) method below is what I come up with. My question is: Should the class allocating memory for my shortened string also be responsible for freeing it? What I do now is to store the returned string in a separate "user class" and defer freeing the memory to that user class.

const char* TextHelper::shortenWithPlaceholder(const char* text, size_t newSize) {
    char* shortened = new char[newSize+1];

    if (newSize <= 3) {
     strncpy_s(shortened, newSize+1, ".", newSize);
    }
    else {
     strncpy_s(shortened, newSize+1, text, newSize-3);
     strncat_s(shortened, newSize+1, "...", 3); 
    }
    return shortened;
}

Answer 1

The standard approach of functions like this is to have the user pass in a char[] buffer. You see this in functions like sprintf(), for example, which take a destination buffer as a parameter. This allows the caller to be responsible for both allocating and freeing the memory, keeping the whole memory management issue in a single place.

Answer 2

In your example the caller has no choice but to be responsible for freeing the allocated memory.

This, however, is an error prone idiom to use and I don't recommend using it.

One alternative that allows you to use pretty much the same code is to change shortened to a referenced counted pointer and have the method return that referenced counted pointer instead of a bare pointer.

Answer 3

Edit: No, I'm wrong. I misunderstood what you were trying to do. The caller must delete the memory in your instance.

The C++ standard states that deleting 0/NULL does nothing (in other words, this is safe to do), so you can delete it regardless of whether you ever called the function at all. Edit: I don't know how this got left out...your other alternative is placement delete. In that case, even if it is bad form, you should also use placement new to keep the allocation/deallocation in the same place (otherwise the inconsistency would make debugging ridiculous).

That said, how are you using the code? I don't see when you would ever call it more than once, but if you do, there are potential memory leaks (I think) if you don't remember each different block of memory.

I would just use std::auto_ptr or Boost::shared_ptr. It deletes itself on the way out and can be used with char*.

Another thing you can do, considering on how TextHelper is allocated. Here is a theoretical ctor:

TextHelper(const char* input) : input_(input), copy(0) { copy = new char[sizeof(input)/sizeof(char)]; //mess with later }
~TextHelper() { delete copy; }

Answer 4

I've never been happy returning pointers to locally allocated memory. I like to keep a healthy mistrust of anyone calling my function in regard to clean up.

Instead, have you considered accepting a buffer into which you'd copy the shortened string?

eg.

const char* TextHelper::shortenWithPlaceholder(const char* text, 
                                               size_t textSize, 
                                               char* short_text, 
                                               size_t shortSize)

where *short_text* = buffer to copy shortened string, and shortSize = size of the buffer supplied. You could also continue to return a const char* pointing to *short_text* as a convenience to the caller (return NULL if shortSize isn't large enough to).

Answer 5

There are two basic ways that I consider equally common: a) TextHelper returns the c string and forgets about it. The user has to delete the memory. b) TextHelper maintains a list of allocated strings and deallocates them when it is destroyed.

Now it depends on your usage pattern. b) seems risky to me: If TextHelper has to deallocate the strings, it should not do so before the user is done working with the shortened string. You probably won't know when this point comes, so you keep the TextHelper alive until the program terminates. This results in a memory usage pattern equal to a memory leak. I'd recommend b) only if the strings belong semantically to the class that provides them, similar to the std::string::c_str(). Your TextHelper looks more like a toolbox that should not be associated with the processed strings, so if I had to choose between the two, I'd go for a). Your user class is probably the best solution, given a fixed TextHelper interface.

Answer 6

The most flexible approach is to return a helper object that wraps the allocated memory, so that the caller doesn't have to worry about it. The class stores a pointer to the memory, and has a copy constructor, an assignment operator and a destructor.

class string_wrapper
{
    char *p;

public:
    string_wrapper(char *_p) : p(_p) { }
    ~string_wrapper() { delete[] p; }

    const char *c_str() { return p; }

    // also copy ctor, assignment
};

// function declaration
string_wrapper TextHelper::shortenWithPlaceholder(const char* text, size_t newSize)
{
    // allocate string buffer 'p' somehow...

    return string_wrapper(p);
}

// caller
string_wrapper shortened = TextHelper::shortenWithPlaceholder("Something too long", 5);

std::cout << shortened.c_str();

Most real programs use std::string for this purpose.

Answer 7

Really you should just use std::string, but if you must, look to the existing library for usage guidance.

In the C standard library, the function that is closest to what you are doing is

char * strncpy ( char * destination, const char * source, size_t num );

So I'd go with this:

const char* TextHelper::shortenWithPlaceholder(
    char * destination, 
    const char * source, 
    size_t newSize);

The caller is responsible for memory management - this allows the caller to use the stack, or a heap, or a memory mapped file, or whatever source to hold that data. You don't need to document that you used new[] to allocate the memory, and the caller doesn't need to know to use delete[] as opposed to free or delete, or even a lower-level operating system call. Leaving the memory management to the caller is just more flexible, and less error prone.

Returning a pointer to the destination is just a nicety to allow you to do things like this:

char buffer[13];
printf("%s", TextHelper::shortenWithPlaceholder(buffer, source, 12));