A bug in code about write a class that is same array of pointer?

Question

Hello to all i have below code
in PointArray.h

#ifndef POINTARRAY_H_INCLUDED
#define POINTARRAY_H_INCLUDED

template <typename T>
class PointArray
{
private:
    int nSize;
    T *array[];
public:
    PointArray();
    PointArray(const T *points[],const int size);
    PointArray(const PointArray &pv);
    ~PointArray();
    int * get(const int position);//is same to array[position]
    const int * get(const int position) const;//is same to array[position]
    const int getSize() const;//get size of array
};

#endif // POINTARRAY_H_INCLUDED

in PointArray.cpp

#include "PointArray.h"
#include<iostream>
#include <assert.h>
using namespace std;

template<class T>
PointArray<T>::PointArray(const T *points[],const int size)
{
    nSize=size;
    for (int i=0;i<size;i++)
    {
        array[i]=new T;
        *array[i]=*points[i];
    }
}

template<class T>
PointArray<T>::PointArray()
{
    nSize=0;
}

template<class T>
PointArray<T>::PointArray(const PointArray &pv)
{
    nSize=pv.getSize();
    for (int i=0;i<nSize;i++)
    {
        array[i]=new T;
        *array[i]=*(pv.get(i));
    }
}

template<class T>
const int PointArray<T>::getSize() const
{
    return nSize;
}

template<class T>
PointArray<T>::~PointArray()
{
    delete[] array;
    nSize=0;
}

template<class T>
int * PointArray<T>::get(const int position)
{
    assert(position>-1 && position<nSize);
    return array[position];
}

template<class T>
const int * PointArray<T>::get(const int position) const
{
    return array[position];
}

in main.cpp

#include<iostream>
#include "PointArray.h"
#include "PointArray.cpp"
using namespace std;

int main()
{
    int x=22;
    int y=3;
    const int * a[2]={&x,&y};
    PointArray<int> p;
    PointArray<int> p2(a,2);
    PointArray<int> p3(p2);
    cout << p.getSize() << endl;
    cout << p2.getSize() << endl;
    cout << p3.getSize() << endl;
    return 0;
}

above code should print

0
2
2

but output is

9244616
9244600
2

and when i run it again 9244616 , 9244600 is changed.
what is problem?

I though you are not setting nSize but later saw in constructor body. Usually it is initialed in initialization list as against in body hence missed to notice. Strange it behave as if nSize is not set to proper value. — Abhijit-K, Apr 05 '13 at 09:35
@MM-BB: there you go.. though I think you need to revisit your memory allocations and members structure — Alon, Apr 05 '13 at 09:42
What is the size of `array` member? Where is it allocated? Your program invokes undefined behavior — Tadeusz Kopec for Ukraine, Apr 05 '13 at 09:45
@MM-BB rewrite your example with just the default constructed object, if that is wrong then you need to understand why. Go from there and use a debugger to understand what is actually happening when it is created - step through it... — Caribou, Apr 05 '13 at 09:48
@MM-BB My mistake - `array` is not allocated. It is only deallocated in destructor (`delete[] array`). Here is one UB you invoke. Another one is accessing `array` elements. How many elements does it have? — Tadeusz Kopec for Ukraine, Apr 05 '13 at 09:58
@MM-BB I have all the original OP code (header, cpp and main) in main.cpp and I get 0 0 2 in vs2012, so it is likely that you are invoking undefined behavior somewhere. UB examples in this : http://stackoverflow.com/questions/367633/what-are-all-the-common-undefined-behaviour-that-a-c-programmer-should-know-ab — Caribou, Apr 05 '13 at 10:16

score 2 · Answer 1 · answered Apr 05 '13 at 10:16

Your problem is

T *array[];

You define an array of pointers with no elements. Then you try to access its (nonexistent) elements which results in undefined behavior.

Another undefined behavior is in destructor

delete[] array;

You should only delete[] things that were new[]-ed. array wasn't. Instead you should loop over all array elements (if there were any) and delete each of them.

The best solution is to use std::vector instead of array. If you insist on using array, change T* array[] to T **array, in constructor allocate memory for it

array = new T*[size];

and in destructor delete all array elements before deleting array itself.

Alon · Accepted Answer · 2013-04-05T10:06:50.943

1

The problem was that you are including the cpp file in the main and implementing there, template implementations should be done in the h files. Your h file should be:

 #ifndef POINTARRAY_H_INCLUDED
#define POINTARRAY_H_INCLUDED

template <typename T>
class PointArray
{
private:
    int nSize;
    T *array[];
public:
    PointArray()
    {
        nSize=0;
    }
    PointArray(const T *points[],const int size)
    {
        nSize=size;
        for (int i=0;i<size;i++)
        {
            array[i]=new T;
            *array[i]=*points[i];
        }
    }
    PointArray(const PointArray &pv)
    {
        nSize=pv.getSize();
        for (int i=0;i<nSize;i++)
        {
            array[i]=new T;
            *array[i]=*(pv.get(i));
        }
    }
    ~PointArray()
    {
        delete[] array;
        nSize=0;
    }
    int * get(const int position)
    {
        assert(position>-1 && position<nSize);
        return array[position];
    }
    const int * get(const int position) const
    {
        return array[position];
    }
    const int getSize() const { return nSize;}
};

#endif // POINTARRAY_H_INCLUDED

Code is still kind of hard and the allocations are bad(notice you get heap assert at the end of the process).. but it worked for me: 0 2 2 is printed... Cheers.

(Notice that:)

delete[] array;

is a memory leak, since your object is not an array per say but you try to define pointers to arrays, you should change it to

T** array

and then at the destructor loop on the array first dimension and then do delete[] array

edited Apr 05 '13 at 10:06

answered Apr 05 '13 at 09:35

Alon

1,776
13
31

Could you clarify your answer a bit, what was wrong with the OP exactly? (Then I'll +1) – Caribou Apr 05 '13 at 09:45
@Alon your answer is for better and code write the code but problem exist – Mahdi-bagvand Apr 05 '13 at 09:48
Edited, thought I was clear, @MM-BB did you try putting it in the headers? it worked for me when I ran it – Alon Apr 05 '13 at 09:54
@Alon unedited code gives me 0 0 2 on vs2012 - it does however emit warnings – Caribou Apr 05 '13 at 09:57
@Caribou yup it is because of the other thing I stated, the T* member[] is a warning, you cannot define it with zero size, it should be T** – Alon Apr 05 '13 at 09:59
@MM-BB from vs2012 : warning C4200: nonstandard extension used : zero-sized array in struct/union Cannot generate copy-ctor or copy-assignment operator when UDT contains a zero-sized array – Caribou Apr 05 '13 at 10:02
@i ran it again and it work but it print `12 [sig] PointArray 3424 open_stackdumpfile: Dumping stack trace to PointArray.exe.stackdump` – Mahdi-bagvand Apr 05 '13 at 10:05
There's no difference between `typename` and `class` in template parameters. – Dan Hulme Apr 05 '13 at 10:05
That might be the memory allocations issues Ive stated, in the last section I try to give a clue – Alon Apr 05 '13 at 10:06
@Alon I haven't downvoted btw – Caribou Apr 05 '13 at 10:09
Doesn't matter, re-editted it to a sentence, it solves the problem @DanHulme thanks for feedback, removed that note – Alon Apr 05 '13 at 10:11

A bug in code about write a class that is same array of pointer?

2 Answers2