Am I deleting this pointer correctly?

Question

I have the following code:

LPWSTR pszDSPath = NULL;
pszDSPath = new WCHAR[  wcslen(pwszFilter)+
                        wcslen(wstrServer.c_str())+
                        wcslen(var.bstrVal) +
                        1
                     ];

// ....
// ....

if(pszDSPath)
{
    delete pszDSPath; 
    pszDSPath = NULL;
}

Can the above code generate a memory leak? I'm not sure if I'm deleting pszDSPath correctly or not.

When you use `new[]` to allocate then you need to use `delete[]` to release memory — Paul R, Feb 17 '11 at 06:54
possible duplicate of [delete vs delete\[\] operators in C++](http://stackoverflow.com/questions/2425728/delete-vs-delete-operators-in-c) — Suma, Feb 17 '11 at 09:22

score 2 · Answer 1 · edited May 23 '17 at 12:20

2

You are not using the correct delete. There are two forms of new: the scalar new that creates a single object (e.g. new int), and the array new that creates an array (e.g. new int[42]).

Likewise, there are two forms of delete: delete and delete[]. If you use new, you must use delete to destroy the object and if you use new[] you must use delete[] to destroy the object.

Since you have used new[] to create the object pointed to by pszDSPath, you must use delete[] pszDSPath to destroy the object.

Note, however, that this would be made much easier if you just used a std::vector:

std::size_t n = wcslen(pwszFilter)+
                wcslen(wstrServer.c_str())+
                wcslen(var.bstrVal) +
                1;

std::vector<WCHAR> v(n);

// &v[0] can be used as you are using pszDSPath in your current code.

In C++, you should eschew manual memory management: it is extraordinarily difficult to get right and it takes a lot of work. There are library facilities in the C++ Standard Library, including containers like std::vector and std::map and smart pointers like std::auto_ptr, std::unique_ptr, and std::shared_ptr, that manage object lifetimes for you. You shouldn't do more work than you have to: if you think you have to write delete somewhere in your code, your code is probably wrong.

This principle of using containers to manage resource lifetimes is based on a design pattern called Scope-Bound Resource Management (SBRM) or Resource Acquisition is Initialization (RAII).

(std::unique_ptr is a part of C++0x designed to replace std::auto_ptr, and your compiler may not yet support it. std::shared_ptr is also a part of C++0x, but it has been available for about a decade as a part of the Boost libraries (as boost::shared_ptr) and was included in C++ TR1.)

edited May 23 '17 at 12:20

Community

1
1

answered Feb 17 '11 at 07:00

James McNellis

348,265
75
913
977

Thanks James for the detailed reply but one point confusing me which Jonathan Wood also mentioned in his comment that for character pointers it doesn't make any difference to delete the pointer either by using "delete pszDSPath" or "delete [] psdDSPath" – RizWiz Feb 17 '11 at 07:35
@RizWiz: That statement in Jonathan Wood's answer is incorrect. You have to correctly match `new[]` with `delete[]`. If you don't, then the behavior of your program is undefined, which means that your program might crash or your data might be corrupted or things might appear to work for months and then suddenly stop working. – James McNellis Feb 17 '11 at 07:38
"Extraordinarily difficult" is a bit exaggerated, isn't it? – Jörgen Sigvardsson Feb 17 '11 at 08:40
@Jörgen: Not at all. It's tedious and difficult in C to manually manage resources, but you don't have much of a choice in C. It's 100 or 1000 times more difficult in C++, which adds non-local control flow (exceptions) and implicitly-called functions (especially constructors, destructors, and overloaded operators). Not only is code that manually manages resources significantly more difficult to write correctly, it is also significantly difficult to verify such code as correct, reason about the code, and modify and maintain it without breaking it. – James McNellis Feb 17 '11 at 15:42
Maybe we have different views on what the word *extraordinarily* means. I agree that it is tedious, and more error prone to keep track of allocated resources "manually" than it is by using some RAII inspired mechanism, but I wouldn't call it *difficult*. Building a robot is what I would classify as difficult. – Jörgen Sigvardsson Feb 17 '11 at 15:47
@Jörgen: If something is error-prone, then doing that something correctly is difficult. If your scale of difficulty is so broad as to encompass all of human endeavor, then really, nothing discussed on this site is really all that difficult. Writing a correct, exception-safe, non-trivial program in C++ without using automatic resource management is very difficult. My rationale for this statement is the large number of bugs that I have found during maintenance, refactoring, and review of legacy code that didn't use automatic resource management. – James McNellis Feb 17 '11 at 16:16

score 1 · Answer 2 · answered Feb 17 '11 at 06:55

1

Use delete[] pszDSPath to avoid memory leaks when you have allocated an array beforehand.

answered Feb 17 '11 at 06:55

Doc Brown

19,739
7
52
88

score 0 · Answer 3 · edited May 23 '17 at 12:20

0

Use delete[] to avoid undefined behavior.

However using delete in you case will almost never cause a memory leak alone - either it will just work or you'll get something much much worse than a memory leak. Don't risk - use delete[].

edited May 23 '17 at 12:20

Community

1
1

answered Feb 17 '11 at 07:32

sharptooth

167,383
100
513
979

Am I deleting this pointer correctly?

3 Answers3