🔒 Locked

C++ class destructor problem

Started by Timkin Jun 24, 2004 at 1:46 AM 43 replies 4k views

Original Post

Jun 24, 2004 at 1:46 AM

Hi everyone... Assume I have a simple augmented template array class that stores the size of the array along with the data. It might look something like this...


template <typename T> class TMyArray
{
   private:
      T*     data;
      size_t length;
   public:
      TMyArray(int L);
      ~TMyArray();

      // ...and typical overloaded operators (. -> [], etc)
}

Now, my array class has (essentially) the following constructor and destructor


//CONSTRUCTOR
template <typename T>
TMyArray<T>::TMyArray(int L)
{
   length = L;
   data = new T[length];
   for (unsigned int i=0;i<length;i++)
      data=0;
}

//DESTRUCTOR
template <typename T>
TMyArray<T>::~TMyArray()
{
    delete [] data;
}

Now, everything works fine and dandy EXCEPT if L=1, in which case the call to the array version of delete throws an exception (which I can't seem to get my Borland debugger to catch 8( ). It doesn't seem to make any difference whether I declare the destructor as virtual or not. What stupid mistake have I made in this. I feel that it is something really obvious, but I'm very tired and rather ill with a virus at the moment and I should be in bed rather than coding... but my software calls... can someone please point out my mistake and ease my suffering? Thanks eternally, Timkin

wild_pointer

Jun 24, 2004 at 1:57 AM

I don't see anything wrong with this, and I compiled it to be sure. What's going on after creation?

Endurion

Jun 24, 2004 at 2:06 AM

Show the code you use to test the class. The class itself shows no errors, but something's missing:

You should also provide a default constructor (no parameters) that initializes data to NULL. Also make sure you don't get the class copied, or better, provide a copy constructor and assignment operator too. Right now, if your class gets assigned and the old instance destroyed data is screwed in the new instance.

Timkin

Jun 24, 2004 at 2:47 AM

Endurion: Sorry for my terse post... those things are of course included... I just wanted to show the essential information (at least, what I was thinking was essential when I wrote it ;) )!

I will throw in one other constructor I use (besides the default constructor, which just initialises data and length to 0)...

template <typename T>TMyArray<T>::TMyArray(T* pData, int L){   length = L;   data = new T[length];   for (unsigned int i=0;i<length;i++)      data = pData;}

The code that tests this is essentially a call to the constructor and a call to the destructor. For example...

int main(){typedef TMyArray<int>  IntArray;IntArray* pTestArray = new IntArray(1);delete pTestArray;return 0;}

Or, if I want to test initialising with another matrix, something like this...

int main(){typedef TMyArray<int>  IntArray;int* pStartArray = new int[1];pStartArray[0]=5;IntArray* pTestArray = new IntArray(pStartArray,1);delete[] pStartArraydelete pTestArray;return 0;}

In actuality, the destructor is called inside another class destructor, however, I get the same error whether I test this directly in my main function or within another destructor. The other class has two of these arrays within it, where one of these arrays is sometimes instantiated with a length of 1. The destructor cleans up the longer array just fine, but craps itself on the shorter array.

I'm using a Borland 5 compiler, which I suspect is the problem if I haven't made a code mistake (which I originally thought I hadn't, since I've used this technique in the past... just not with the convolution of class structures I have in the current project).

The debugger picks up that an exception is thrown when it attempts to execute the line delete [] data in the destructor when L=1 in the constructor. I'm having trouble with catching the exception because Borland doesn't use standard exception handling... at least, not in that my compiler.

Thanks for taking the time to help.

Timkin

snk_kid

Jun 24, 2004 at 2:55 AM

There isn't anything wrong with the code shown so far but you should post all your code, a couple of things i would suggest thou first being that a constructor to specifiy the size of its container you should tag it with the keyword "explicit" so you don't get implicit conversions with assignement.

Also you i would give a default value to "L" of 0 so it becomes a default constructor aswell, use constructor initializers instead of set members inside the construtor.

Make sure you provide an appropriate copy & assignement operators other wise you'll get a default copy of all memebers so you'll end up having pointers pointing to the same object and you'll either get delete called twice or one of the pointers will become invalid.

This is how i would change things:

array.hpp

#include <cstddef>#include <new>template< typename T >class array {public:   typedef size_t    size_type;   typedef ptrdiff_t difference_type;private:   T* _a;   size_type _size;public:   explicit array(size_type n = 0): _size(n), _a(0) {         try {               _a = new T[_size];         } catch(const std::bad_alloc& err) {             //possibly rethrow the exception for             //user to handle         }   }   array(const array< T >& a): _size(a._size), _a(0) {      try {           _a = new T[_size];            for(difference_type i = 0, _n = a._size;                i < n;                ++i) {                _a = a._a;             }      } catch(const std::bad_alloc& err) {             //possibly rethrow the exception for             //user to handle      }   }   void clear() {      if(_a != 0) {          delete[] _a;          _a = 0;          _size = 0;      }   }   array<T>& operator=(const array<T>& ar) {     if(this != &ar) {         clear();         _size = ar._size;        try {              _a = new T[_size];             for(difference_type i = 0, _n = ar._size;                 i < n;                 ++i) {                 _a = ar._a;             }         } catch(const std::bad_alloc& err) {             //possibly rethrow the exception for             //user to handle         }      }      return *this;   }   ~array() {       clear();    }};

That being said from what your trying to achieve you'll be better of using the standard library vector.

SiCrane

Jun 24, 2004 at 2:56 AM

I ran your code through two different Borland compilers on my computer and neither of them duplicated the behaviour you described. It also looks all perfectly kosher (modulo typos).

Timkin

Jun 24, 2004 at 3:01 AM

Quote:
Original post by snk_kid
There isn't anything wrong with the code shown so far but you should post all your code

Unfortunately I cannot because of intellectual property constraints... the code is being written as part of my job and I cannot disclose the actual code... I can 'talk around it' though... sorry that this makes it harder to solve the problem.

Quote:
Original post by snk_kid
a couple of things i would suggest thou first being that a constructor to specifiy the size of its container you should tag it with the keyword "explicit" so you don't get implicit conversions with assignement.

Also you i would give a default value to "L" of 0 so it becomes a default constructor aswell, use constructor initializers instead of set members inside the construtor.

Make sure you provide an appropriate copy & assignement operators other wise you'll get a default copy of all memebers so you'll end up having pointers pointing to the same object and you'll either get delete called twice or one of the pointers will become invalid.

All of these things are of course in my actual code... I'm just trying to simplify the problem above by putting in only the relevant information (since I cannot post my code).

Quote:

That being said from what your trying to achieve you'll be better of using the standard library vector.

Unfortunately the STL vector class is a little too fat for my application (too much overhead to store short arrays)... and I'm storing LOTS of arrays. Besides, I don't need dynamic resizing of the arrays once they're instantiated. I could get around my problem by just using regular arrays and keeping track of (or computing) sizes every time I needed them... but that's a pain. It's nice having a size() method to call on when its needed.

Cheers,

Timkin

Timkin

Jun 24, 2004 at 3:02 AM

Quote:
Original post by SiCrane
It also looks all perfectly kosher (modulo typos).

Hehe.. they were fixed while you were running it (and I was editing my post)!

hehe

snk_kid

Jun 24, 2004 at 3:08 AM

Quote:
Original post by Timkin
Quote:
Original post by snk_kid
There isn't anything wrong with the code shown so far but you should post all your code

Unfortunately I cannot because of intellectual property constraints... the code is being written as part of my job and I cannot disclose the actual code... I can 'talk around it' though... sorry that this makes it harder to solve the problem.

Quote:
Original post by snk_kid
a couple of things i would suggest thou first being that a constructor to specifiy the size of its container you should tag it with the keyword "explicit" so you don't get implicit conversions with assignement.

Also you i would give a default value to "L" of 0 so it becomes a default constructor aswell, use constructor initializers instead of set members inside the construtor.

Make sure you provide an appropriate copy & assignement operators other wise you'll get a default copy of all memebers so you'll end up having pointers pointing to the same object and you'll either get delete called twice or one of the pointers will become invalid.

All of these things are of course in my actual code... I'm just trying to simplify the problem above by putting in only the relevant information (since I cannot post my code).

Quote:

That being said from what your trying to achieve you'll be better of using the standard library vector.

Unfortunately the STL vector class is a little too fat for my application (too much overhead to store short arrays)... and I'm storing LOTS of arrays. Besides, I don't need dynamic resizing of the arrays once they're instantiated. I could get around my problem by just using regular arrays and keeping track of (or computing) sizes every time I needed them... but that's a pain. It's nice having a size() method to call on when its needed.

Cheers,

Timkin

Okay so you wont a static array type well i've written one that you can use with STL algorithms & i write a special iterator that does bounds checking but i haven't done much testing with that the array types works thou you can have it:

static_array.hpp

#ifndef _STATIC_ARRAY_#define _STATIC_ARRAY_#include <ostream>#include <istream>#include <iterator>#include <stdexcept>#include <string>#include <algorithm>#include <cstddef>namespace utility {	class out_of_bounds;	template< class container, class itr > class checked_iterator;	template< class T, const ::size_t n > class static_array;	template< class T, const ::size_t n > std::ostream& operator<<(std::ostream&, const static_array< T, n >&);	template< class T, const ::size_t n > std::istream& operator>>(std::istream&, static_array< T, n >&);class out_of_bounds : public std::out_of_range {public:	out_of_bounds(): out_of_range("iterator out of bounds exception") {};};template< class container, class itr = typename container::iterator >class checked_iterator : public std::iterator_traits< container > {	itr curr;	container* c;public:	void validate(itr p) const throw(out_of_bounds) {		if(c->end() == p) return;		for(itr pp = c->begin(); pp != c->end(); ++p)			if(pp == p) return;		throw out_of_bounds();					}	const bool operator==(const checked_iterator< container, itr >& chk_it) const {		return ((c == chk_it.c) && (curr == chk_it.curr));	}	checked_iterator(const checked_iterator& x, itr p) throw(out_of_bounds): c(&x), curr(p) {		validate(p);	}	checked_iterator(const checked_iterator& x): c(&x), curr(x.begin()) {}	const typename std::iterator_traits< container >::reference 		operator*() const throw(out_of_bounds) {			if(curr == c->end())				throw out_of_bounds();			return *curr;		}	typename std::iterator_traits< container >::reference		operator*() throw(out_of_bounds) {			if(curr == c->end())				throw out_of_bounds();			return *curr;		}	const typename std::iterator_traits< container >::pointer 		operator->() const throw(out_of_bounds) {			if(curr == c->end())				throw out_of_bounds();			return &*curr;		}	typename std::iterator_traits< container >::pointer		operator->() throw(out_of_bounds) {			if(curr == c->end())				throw out_of_bounds();			return &*curr;		}	const checked_iterator< container, itr >&		operator+(typename std::iterator_traits< container >::difference_type d) const throw(out_of_bounds) {			if(((c->end() - curr) < d) || (d < (-(curr - c->begin()))))				throw out_of_bounds();			return checked_iterator< container, itr >(c, curr+d);		}	const typename std::iterator_traits< container >::reference		operator[](typename std::iterator_traits< container >::difference_type d) const throw(out_of_bounds) {			if(((c->end() - curr) <= d) || (d < (-(curr - c->begin()))))				throw out_of_bounds();			return curr[d];		}	typename std::iterator_traits< container >::reference		operator[](typename std::iterator_traits< container >::difference_type d) throw(out_of_bounds) {			if(((c->end() - curr) <= d) || (d < (-(curr - c->begin()))))				throw out_of_bounds();			return curr[d];		}	const checked_iterator< container, itr >& operator++() throw(out_of_bounds) {		if(curr == c->end())			throw out_of_bounds();		++curr;		return *this;	}	const checked_iterator< container, itr >& operator++(int) throw(out_of_bounds) {		checked_iterator< container, itr > tmp = *this;		++*this;		return tmp;	}	const checked_iterator< container, itr >& operator--() throw(out_of_bounds) {		if(curr == c->begin())			throw out_of_bounds();		--curr;		return *this;	}	const checked_iterator< container, itr >& operator--(int) throw(out_of_bounds) {		checked_iterator< container, itr > tmp = *this;		--*this;		return tmp;	}	typename std::iterator_traits< container >::difference_type index() const {		return (curr - c->begin());	}	itr unchecked() const { return curr; }};template<class T, const ::size_t n> class static_array {public:	typedef T value_type;    typedef value_type* pointer;    typedef const value_type* const_pointer;    typedef T* iterator;    typedef const T* const_iterator;		typedef std::reverse_iterator<const_iterator>	const_reverse_iterator;    typedef std::reverse_iterator<iterator>			reverse_iterator;    typedef value_type& reference;    typedef const value_type& const_reference;    typedef ::size_t size_type;	typedef const ::size_t const_size_type;    typedef ::ptrdiff_t difference_type;	typedef const ::ptrdiff_t const_difference_type;	static_array() {};        	static_array(const static_array< T, n >& a)			{	std::copy(a.begin(), a.end(), this->begin());	}	const static_array& operator=(const static_array< T, n >& a)	{	std::copy(a.begin(), a.end(), this->begin()); return *this;	}	operator T*()				{	return _n;	}	operator const T*() const	{	return _n;	}	bool operator==(const static_array& a) const			{	return std::equal(this->begin(), this->end(), a.begin());	} 	bool operator<(const static_array< T, n >&  a) const	{	return std::lexicographical_compare(a.begin(), a.end(), this->begin(), this->end());	}	bool operator<=(const static_array< T, n >& a) const	{	return !(*this < a);	}	const bool operator>(const static_array< T, n >& a) const	{	return a < *this;	}	bool operator>=(const static_array< T, n >& a) const	{	return !(a < *this);	}	bool operator!=(const static_array< T, n >& a) const	{	return !(*this == a);	}	const_reference operator[](difference_type index) const	{	return _n[index];	}	reference operator[](difference_type index)				{	return _n[index];	}	const_iterator begin() const	{	return _n;	}	iterator begin()				{	return _n;	}	const_iterator end() const		{	return (_n + _size);	}	iterator end()					{	return (_n + _size);	}	const_reverse_iterator rbegin() const	{ return const_reverse_iterator(end());	}	reverse_iterator rbegin()				{ return reverse_iterator(end()); }	const_reverse_iterator rend() const		{ return const_reverse_iterator(begin()); }	reverse_iterator rend()					{ return reverse_iterator(begin()); }	const_reference front() const			{	return _n[0]; }	reference front()						{	return _n[0];	}	const_reference back() const			{	return _n[_size - 1];	}	reference back()						{	return _n[_size - 1];	}	size_type max_size() const	{	return _size;	}	size_type size() const		{	return _size;	}	bool empty() const			{	return	false;	}	void swap(static_array< T,n >& y)	{	std::swap_ranges(begin(), end(), y.begin());	}private:	static const_size_type _size	=	n;	value_type _n[_size];};template<class T, const ::size_t n>inline std::ostream& operator<<(std::ostream& out, const static_array< T, n >& a) {	std::ostream_iterator< T > out_itr(out, "\n");	std::copy(a.begin(), a.end(), out_itr);	return out;}template<class T, const ::size_t n>inline std::istream& operator>>(std::istream& in, static_array< T, n >& a) {	std::copy(std::istream_iterator< T >(in), // beginning of source         	std::istream_iterator< T >(),		// end of source		a.begin());							// destination	return in;}};#endif

[Edited by - snk_kid on June 24, 2004 5:08:27 AM]

Timkin

Jun 24, 2004 at 3:09 AM

Quote:
Original post by SiCrane
I ran your code through two different Borland compilers

Which compilers?

SiCrane

Jun 24, 2004 at 3:10 AM

Quote:
Original post by Timkin
Hehe.. they were fixed while you were running it (and I was editing my post)!

Nope, you're still missing a semi-colon at the end of your template class declaration, :)

In any case, if you want something a little lighter than a vector, you might want to consider Andrei Alexandrescu's typed buffer that he describes in an article in CUJ. Typed Buffers(I).

edit: BCC 5.5, from their free command line tools and Borland C++ Builder 4.

Timkin

Jun 24, 2004 at 3:11 AM

Thanks for the offer snk_kid... I'm heading home now and I'll take a look at your code when I get home... I would like to know why mine is have the problem... but if I can't work it out, then I might come a knockin'...

Thanks,

Timkin

certaintragedy

Jun 24, 2004 at 3:46 PM

Maybe I just have a gross misunderstanding of the nature of this problem, but can't you just say...

if(length == 1)
delete data;
else
delete [] data;

... or am I just confused?

Muhammad Haggag

Jun 24, 2004 at 4:01 PM

Quote:
Original post by certaintragedy
Maybe I just have a gross misunderstanding of the nature of this problem, but can't you just say...

if(length == 1)
delete data;
else
delete [] data;

... or am I just confused?

Well, the point is he allocated the data using:

data = new T[length];

Which calls vector new (was that the name?), which, by definition, allocates memory that should be deleted by vector delete.

Vector new stores the number of items allocated somewhere around the actual memory block requested (implementation dependent), and vector delete makes use of that info. So even on unity allocations, a count of "1" should be stored, and vector delete should understand that and call the destructor of 1 object only...

If what you suggest works, however, then it's a compiler bug.

CoffeeMug

Jun 24, 2004 at 4:04 PM

What sort of class do you pass as a template parameter? May be it overrides operator delete[] and doesn't do it correctly?

Did you try the previous solution (testing if L == 1 and using plain delete in that case)?

RenderTarget

Jun 24, 2004 at 4:06 PM

Could it be a bad library version? I can't imagine they'd do this, but it might be that the memman library you have treats a 1-element alloc like a non-array allocation. ?

Maybe step into the new call and see what it's doing.

wild_pointer

Jun 24, 2004 at 4:14 PM

Does this run without error?

template <typename T>class TMyArray{    size_t length;    T* data;public:    TMyArray(int L)    {        length = L;        data = new T[length];        for (unsigned int i=0;i<length;i++)            data=0;    }    ~TMyArray()    {        delete [] data;    }};int main(void){    TMyArray<char> *a = new TMyArray<char>(1);    delete a;}

You could at least narrow down the problem from there.

Timkin

Jun 24, 2004 at 7:54 PM

Thanks everyone for the input... I've confirmed that it is definitely a compiler problem... I was able to compile my code using MSVC++6 without too many issues (I should say I'm amazed at the non-standard way that MSVC++ handles templates... but then, it's Microsoft, so I shouldn't be surprised!)!

So, it's definitely a problem with my Borland 5 compiler...

I'll play around with it a little (in particular, the instantiation). I vaguely recall testing the use of delete data for the case dimension==1 and I don't recall it working... but I'll check it again and confirm the result.

Anyway, I do appreciate the replies and the testing people did.

Thanks heaps,

Timkin

CoffeeMug

Jun 24, 2004 at 8:08 PM

Quote:
Original post by Timkin
I should say I'm amazed at the non-standard way that MSVC++ handles templates... but then, it's Microsoft, so I shouldn't be surprised!

You should try MSVC 2003. It's so good, you'd think it was released by another company [smile]

Shannon Barber

Jun 24, 2004 at 9:49 PM

The Borland compilers are very good, how certain are you it's a compiler problem? Such a simple use of delete[] is unlikely to be bugged. I'd suspect heap corruption.

And of course you know you should using std::vector :P

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.