[RESOLVED] Why does this code leak memory?

[Brownhead]

// Check to see if the Found array needs to be expanded
if (Entry > Max)
{
    // Give the array 30 more spaces to fill
    Max += 30;

    /* Create a temp array with the amount of space desired,
        if the memory cannot be allocated return an error */
    Temp = new POINT[Max];
    if (!Temp)
    {
        delete [] Found;
        LastERR = FAILED_MALLOC;
        return NULL;
    }

    // Loop through the original and copy it to the new array
    for (int iCountARY = 0; iCountARY < Entry; iCountARY++)
    {
        Temp[iCountARY] = Found[iCountARY];
    }

    // Release the old array
    delete [] Found;
    Found = Temp;
}

I'm a beginner to C++ and am trying to check if a dynamic array needs to be expanded, and if so, expand it. But my compiler (Dev-C++) is giving me this warning

warning: heap block at 003F3CF8 modified at 003F3DF0 past requested size of f0

Which I think, after a little research, means that I was trying to access memory outside of my dynamic array or something along those lines. If anyone could quickly go over what I'm doing wrong I'd really appreciate it, thanks for reading!

[souldog]

arrays are zero indexed, so your conditional should be

if (Entry >=  Max)

since your are allocating an array of size MAX.

I am not sure if that was clear so

POINT[5], contains 5 POINTS which you access with indices
0, 1, 2, 3, 4

using 5 for access will give the error you see

[GNiewerth]

It doesn´t leak memory, you are accessing a memory location beyond array bonds. This will happen if (Entry +30) is larger than Max, because you resize your array by 30 instead of looking how many elements you´re gonna need. If you replace the line

// Give the array 30 more spaces to fill
Max += 30;

by

[code]
// adjust array size to hold Entry elements
Max = Entry;
[/b]

or

while( Entry > Max )
{
   // resize array until it´s large enough to hold Entry elements
   Max = Max * 2;
}

Maybe you can use std::vector instead?

[Paul McKenzie]

I'm a beginner to C++ and am trying to check if a dynamic array needs to be expanded, and if so, expand it.

Use std::vector. Not only is it safer, it is much smarter and faster than using naked new[]/delete[] calls to do this.

#include <vector>

int main()
{
   std::vector<int> IntArray(10);  // an array of 10 integers.
   IntArray.resize(20);  // now it has 20 integers
   IntArray[0] = 10;  // use just like a regular array
}

There is your entire code in two lines.

Also, as mentioned before, what you're doing is slow compared to using vector and resize(). Believe me. I've timed code that looks like yours, and vector::resize() as opposed to using new[]/delete[] is many times faster because resize() has more intelligence than just doing straight new[] and delete[] calls each time the size changes.

Also, operator new[] does not return NULL if there is a failure. A conforming C++ compiler throws a std::bad_alloc exception if new fails.

Regards,

Paul McKenzie

[MikeAThon]

Temp = new POINT[Max];
...

for (int iCountARY = 0; iCountARY < Entry; iCountARY++)

In addition to the above, you are only allocating "Max", but you are allowing the subscript to exceed "Max" since you compare against "Entry".

Change to

Temp = new POINT[Entry];

Mike

[Brownhead]

Thanks everyone! I'll just use vectors, it looks to be alot simpler because I have many dynamic arrays in my code and handling them has been a bit of a chore. Thanks again everyone!

[JamesSchumacher]

It is better to properly learn to use new[]/delete[] properly than to live on the crutch of std::vector though.

std::vector requires the use of the STL new operator, which is defined to throw an exception on failure. It include <new> which defines it to throw a std::bad_alloc, STL expects this.

You cannot override it, if you try - you will get an error because it is not defined to be the same as the first declaration. (That of which is in <new>)

And exceptions are a lot of overhead, for the simple fact they are no good for library code based on the simple fact people can use that code 10 function calls deep, and have to throw it all the way to the GUI. This is not acceptable! (Or they think they have to)

Don't just tell someone to use std::vector, you can recommend it - however, if you are going to tell someone the advantages of something, if there are big performance and design issues (there are big ones with STL people do not admit) then they should be aware of that as well.

[Paul McKenzie]

It is better to properly learn to use new[]/delete[] properly than to live on the crutch of std::vector though.

That "crutch" you speak of is responsible for less bugs and faster development time.

Why do you call it a "crutch"? It sounds like you're calling programmers who use std::vector and other containers novices because we don't know how to use new[]/delete[]. Myself and others here that use vector already know how to use new[]/delete[]. Even the OP knew how to use new/delete, so it isn't a matter of not knowing how to use it.

Heck, wherever new[] is called, a corresponding delete[] on the pointer value returned must be done. Big deal. But the problem is that these operations are done in pairs, and each one of in that pair could be in who knows what part of the program, thereby causing a logical disconnect in the code between the new/delete calls. To try and make this connection requires coding that does all sorts of contortions, with no real confidence that the code is really sound for all conditions. I've seen it myself, and it isn't pretty.

The problem is that C++ programmers are tired of maintaining code that does the new[]/delete[] gymnastics that the OP posted (i.e. logical spaghetti), and many code reviewers would reject the code that was posted because of this. I personally had to debug code that looked like the OP's (where to delete, when to new, who's responsible for what, what if this happened, etc.). In short, a maintenance nightmare -- all of the code I had to debug could have been replaced with a vector and resize().

std::vector requires the use of the STL new operator, which is defined to throw an exception on failure. It include <new> which defines it to throw a std::bad_alloc, STL expects this.

You cannot override it,

Yes you can, by creating your own allocator. That's what the second template parameter is there for. Anyway, take 1,000 experienced C++ programmers -- how many of them really look to this as a problem?

Don't just tell someone to use std::vector,

Too late. The OP is now using vector, and seems to be making progress. Anyway, it's a C++ forum, and I gave a C++ answer.

Also, you can recommend it - however, if you are going to tell someone the advantages of something, if there are big performance and design issues (there are big ones with STL people do not admit)

And what are these issues with vector, and who are these people that you speak of? I don't know of any issues, which is why I need not mention them. You mention some pathological case concerning some GUI, but anyone can come up with the odd case for anything to do with the language that >99% of programmers will not care about.

Also, I've already mentioned that manually using straight new[]/delete[], especially if the pointer variable is a member of a class (similar to the OP's code), is demonstrably slower than just using a vector as a member and using resize() when appropriate.

Given 1,000 programmers, will the vast majority choose more speed, quicker development time, and easy maintenance, or be concerned by operator new throwing an exception (which is rare anyway), thereby coding a nightmare that would scare off any programmer that may need to maintain the code and also risk slower execution (and more bugs)?

If the OP were using vector, more than likely he never would have posted anything on CodeGuru, and instead got his real work done.

http://www.parashift.com/c++-faq-lite/containers.html

Regards,

Paul McKenzie

[JamesSchumacher]

Some source from a rolled class. You want to see why I do not use vector? Have fun. Let's see vector do something like this. Are you going to implement using a vector as the underlying container for a ListBox? Hah...

This is the real world buddy. Check out where it says RaiseContentChanged().

bool ListItemArray::SetCapacity(unsigned long dwCapacity,bool bKeepData)
    {
        if (bKeepData == false)
        {
            Clear();
        }

        if (dwCapacity > m_dwCapacity)
        {
            ListItem ** ppNewData = new ListItem*[dwCapacity];

            if (ppNewData == 0)
            {
                return false;
            }

            if (m_dwCount != 0)
            {
                ListItem ** ppDest = ppNewData;
                ListItem ** ppSource = m_ppListItems;
                ListItem ** ppEnd = ppSource + m_dwCount;

                while (ppSource != ppEnd)
                {
                    *ppDest++ = *ppSource++;
                }
            }

            if (m_ppListItems != 0)
            {
                delete [] m_ppListItems;
            }

            m_ppListItems = ppNewData;
            m_dwCapacity = dwCapacity;
        }

        return true;
    }

    bool ListItemArray::SetLength(unsigned long dwLength)
    {
        if (dwLength > m_dwCount)
        {
            if (dwLength > m_dwCapacity)
            {
                const unsigned long dwAlloc = DetermineAlloc(dwLength);

                if (!SetCapacity(dwAlloc,true))
                {
                    return false;
                }
            }

            ListItem ** ppStart = &m_ppListItems[m_dwCount];
            ListItem ** ppEnd = &m_ppListItems[dwLength];

            while (ppStart != ppEnd)
            {
                *ppStart = new ListItem;
                ++ppStart;
            }

            m_dwCount = dwLength;

            if (m_bUpdating == false)
            {
                RaiseContentChanged();
            }
        }
        else if (dwLength < m_dwCount)
        {
            for (; m_dwCount != dwLength; --m_dwCount)
            {
                delete m_ppListItems[m_dwCount - 1];
                m_ppListItems[m_dwCount - 1] = 0;
            }

            if (m_bUpdating == false)
            {
                RaiseContentChanged();
            }
        }

        return true;
    }

    void ListItemArray::Clear()
    {
        if (m_dwCount != 0)
        {
            ListItem ** ppStart = m_ppListItems;
            ListItem ** ppEnd = ppStart + m_dwCount;

            while (ppStart != ppEnd)
            {
                delete *ppStart;
                *ppStart = 0;
                ++ppStart;
            }

            m_dwCount = 0;

            if (m_bUpdating == false)
            {
                RaiseContentChanged();
            }
        }
    }

    bool ListItemArray::Add(ListItem * pItem)
    {
        if (pItem != 0)
        {
            const unsigned long dwNewLength = m_dwCount + 1;

            if (dwNewLength > m_dwCapacity)
            {
                const unsigned long dwAlloc = DetermineAlloc(dwNewLength);

                if (!SetCapacity(dwAlloc,true))
                {
                    return false;
                }
            }

            m_ppListItems[m_dwCount] = pItem;
            ++m_dwCount;

            if (m_bUpdating == false)
            {
                RaiseContentChanged();
            }

            return true;
        }

        return false;
    }

    bool ListItemArray::AddString(const AnsiString & str)
    {
        ListItem * pItem = new ListItem;

        if (pItem != 0)
        {
            pItem->SetText(str);

            const bool bCheck = Add(pItem);

            if (bCheck == false)
            {
                delete pItem;
            }

            return bCheck;
        }

        return false;
    }

    void ListItemArray::RaiseContentChanged()
    {
        ListItemArrayEventArgs * pArgs = new ListItemArrayEventArgs;
        pArgs->Sender = this;
        pArgs->Count = m_dwCount;

        ContentChanged.Raise(pArgs);

        delete pArgs;
    }

    unsigned long __stdcall ListItemArray::DetermineAlloc(unsigned long dwCount)
    {
        unsigned long dwBlockCount = dwCount / BlockSize;

        if (dwBlockCount == 0 || (dwCount % BlockSize) != 0)
        {
            ++dwBlockCount;
        }

        return dwBlockCount * BlockSize;
    }

Do you understand WHY though your statement about std::vector is that's its faster? It's only because of iterator access (which for most types is just a pointer). (And it's method of not destroying objects under reallocation)

If someone uses pointer access like I do inside my newer classes, my classes run circles around vector.

Consider that fact most people use for loops, this type of loop guarantees atleast an implicit mulitplication from the integer iterator to get the correct offset. This is very slow compared to direct pointer dereference.

In the latter, you have one multiplication of this, and then all increment/decrements. That is the only thing that makes vector fast.

So you referring to a standard paper of sorts explains nothing. The actual binary code generated from a certain algorithm dictates how fast it is.

Pic Link

So you stand by the standard just because it's standard, (however, going nowhere) - get your heads out of your arse and wake up.

[laserlight]

Out of curiosity, but wouldn't it have been simpler to write this:

void ListItemArray::RaiseContentChanged()
{
    ListItemArrayEventArgs * pArgs = new ListItemArrayEventArgs;
    pArgs->Sender = this;
    pArgs->Count = m_dwCount;

    ContentChanged.Raise(pArgs);

    delete pArgs;
}

as this?

void ListItemArray::RaiseContentChanged()
{
    ListItemArrayEventArgs pArgs;
    pArgs.Sender = this;
    pArgs.Count = m_dwCount;

    ContentChanged.Raise(&pArgs);
}

Maybe I am missing something really obvious and serious, but it also seems to me that a vector<ListItem*> or vector<tr1::shared_ptr<ListItem> > boost ptr_vector<ListItem> could indeed be used as the underlying container. I do not understand: what makes RaiseContentChanged() so special?

[JamesSchumacher]

As for the dynamic issue... Stack variables cannot be passed to another thread. If the handlers invoked in this thread, set an event handle (synchro event) and pass the args to the new thread, well... It has to be on the heap now doesn't it?

As for RaiseContentChanged(), you raise an event to the listbox so it can recalculate it's draw data and repaint if it's not 'updating'.

Or whatever wants notified when the contents of the container change. If you do this with a wrapper of std::vector, then you defeat the purpose that you are trying to portray about vector and it's performance.

So it is a fair comparison. Sure, RaiseContentChanged() adds overhead to the container itself, but it does so in a way that exposes FUNCTIONALITY... In which a wrapper of a vector does not to the vector itself.

Believe me my friends... STL touched on this base with function objects... It didn't go far enough. And it has it's downfalls (STL).

[Paul McKenzie]

Some source from a rolled class.

Pathological case.

You want to see why I do not use vector?

So just dismiss anything I stated to you about 99% of programmers not having any issues with using vector appropriately.

Have fun. Let's see vector do something like this. Are you going to implement using a vector as the underlying container for a ListBox? Hah...
...
This is the real world buddy.
....
get your heads out of your arse and wake up

This isn't the World Wrestling Entertainment. Please be more mature in your responses. The last response could get you banned here. Also, it looks like you are trying to imply that vector was supposed to be some uber component that didn't work for you. It's purpose is as a generic dynamic array -- that's it.

Also, I deal with the real world, as all of my programs are used by thousands of persons. I don't know who uses your programs or code.

Also, the OP has no problems with using vector, as do many thousands of programmers.

Regards,

Paul McKenzie

[laserlight]

As for the dynamic issue... Stack variables cannot be passed to another thread. If the handlers invoked in this thread, set an event handle (synchro event) and pass the args to the new thread, well... It has to be on the heap now doesn't it?

I have no real experience in threads (hobbyist programmer with specific hobbies, you understand), but that sounds like a good explanation.

As for RaiseContentChanged(), you raise an event to the listbox so it can recalculate it's draw data and repaint if it's not 'updating'.

Or whatever wants notified when the contents of the container change. If you do this with a wrapper of std::vector, then you defeat the purpose that you are trying to portray about vector and it's performance.

Ah, so it is not that you are arguing that it is impossible, but that it is technically inferior performance-wise to wrap a vector in your case?

[Paul McKenzie]

Ah, so it is not that you are arguing that it is impossible, but that it is technically inferior performance-wise to wrap a vector in your case?

Which means we would have timing tests, comparisons, and actual code showing the vector implementation, all without the vitriol and bravado.

Regards,

Paul McKenzie

[JamesSchumacher]

I have no real experience in threads (hobbyist programmer with specific hobbies, you understand), but that sounds like a good explanation.


Ah, so it is not that you are arguing that it is impossible, but that it is technically inferior performance-wise to wrap a vector in your case?

Exactly. In C++, just about anything is possible. Even manual VTable implementation.