Jump to content

  • Log In with Google      Sign In   
  • Create Account

#ActualHodgman

Posted 15 September 2013 - 11:39 PM

Yes, all the std:: containers work well with any data type.

There are some issues with your container though:

1)
ZeroMemory(&Data, sizeof(DataT));
If DataT is a non-POD class (if it has constructors, or members that have constructors), then this is very, very bad. Just delete that line.
 
e.g. in the example below, std::vector is a non-POD class. It has constructors/destructors that perform careful memory management. On line #1, this constructor allocates 42 integers.
On line #2, this vector is overwritten with zeros... This is a really bad error. In the best case, maybe your program won't crash right away, and you've simply corrupted the object.
On line #3, we try to use the corrupted object, and this will likely crash.
std::vector<int> myVector(42); // #1
ZeroMemory(&myVector, sizeof(std::vector<int>)); // #2
myVector[12] = 12; // #3
 
2)
 Your other constructor isn't wrong, but it's very inefficient:
/// old
	CNode(DataT d)//copying 'd' by value here, means that the input class is copied from it's original location to 'd'
	{//hidden here: C++ default-constructs all members
		Data = d;//after 'Data' has been default-constructed, then copy d over the top of it
		Next = NULL;
		Prev = NULL;
	}
///new
	CNode(const DataT& d) // don't pass 'd' by value, pass by reference to avoid making an extra copy
	 : Data(d) // instead of default-constructing Data, use a copy-constructor
	{
		Next = NULL;
		Prev = NULL;
	}
 
3) You wrote that constructor above that takes a DataT argument, but don't even use it:
/// old
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(DataT *d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>;//default constructs 'Data', then uses ZeroMemory over it
	NumNodesAllocated++;

	pNewNode->Data = *d; // then copies '*d' over 'Data' again

/// new
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(const DataT& d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>(d);//construct 'Data' just once, using the right value
	NumNodesAllocated++;
Other issues:

4) This has no place in a linked list class :-P
It has no impact on your question (whether classes work), but the average user would be surprised to find that the global srand value was being changed whenever they create a linked list.
srand(GetTickCount());
5) Why does CLinkedList inherit from CNode?

6) Your CLinkedList class violates the rule of 3 / rule of 2, which will cause code like below to crash:
{
  int value = 42;
  CLinkedList<int> myList;
  myList.Push( &value );
 
  CLinkedList<int> myOtherList = myList; // at the moment, this will perform a shallow copy
  //both lists now point to the same nodes!
  //when they're destructed, they will both try to delete these same nodes!
}//crash!!! double deletion bug!
You can fix this in two ways:
A) make CLinkedList non-copyable:
template <class DataT> class CLinkedList {	
private:
  CLinkedList( const CLinkedList& );
  CLinkedList& operator=( const CLinkedList& );
  //if anyone tries to copy a CLinkedList now, they'll get a compile-time error saying that copying is private
...
B) Implement copying
template <class DataT> class CLinkedList {
...	
public:
...
  CLinkedList( const CLinkedList& otherList )
  {
    First = NULL;
    Last  = NULL;
    NumNodesAllocated = 0;
    *this = otherList;
  }
  CLinkedList& operator=( const CLinkedList& otherList )
  {
    Clear();
    for each node in otherList //pseudo code
      this.Push(node.Data);
    return *this;
  }
...

#4Hodgman

Posted 15 September 2013 - 11:37 PM

Yes, all the std:: containers work well with any data type.

There are some issues with your container though:

1)
ZeroMemory(&Data, sizeof(DataT));
If DataT is a non-POD class (if it has constructors, or members that have constructors), then this is very, very bad. Just delete that line.
 
e.g. in the example below, std::vector is a non-POD class. It has constructors/destructors that perform careful memory management. On line #1, this constructor allocates 42 integers.
On line #2, this vector is overwritten with zeros... This is a really bad error. In the best case, maybe your program won't crash right away, and you've simply corrupted the object.
On line #3, we try to use the corrupted object, and this will likely crash.
std::vector<int> myVector(42); // #1
ZeroMemory(&myVector, sizeof(std::vector<int>)); // #2
myVector[12] = 12; // #3
 
2)
 Your other constructor isn't wrong, but it's very inefficient:
/// old
	CNode(DataT d)//copying 'd' by value here, means that the input class is copied from it's original location to 'd'
	{//hidden here: C++ default-constructs all members
		Data = d;//after 'Data' has been default-constructed, then copy d over the top of it
		Next = NULL;
		Prev = NULL;
	}
///new
	CNode(const DataT& d) // don't pass 'd' by value, pass by reference to avoid making an extra copy
	 : Data(d) // instead of default-constructing Data, use a copy-constructor
	{
		Next = NULL;
		Prev = NULL;
	}
 
3) You wrote that constructor above that takes a DataT argument, but don't even use it:
/// old
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(DataT *d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>;//default constructs 'Data', then uses ZeroMemory over it
	NumNodesAllocated++;

	pNewNode->Data = *d; // then copies '*d' over 'Data' again

/// new
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(const DataT& d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>(d);//construct 'Data' just once, using the right value
	NumNodesAllocated++;
Other issues:

4) This has no place in a linked list class :-P
It has no impact on your question (whether classes work), but the average user would be surprised to find that the global srand value was being changed whenever they create a linked list.
srand(GetTickCount());
5) Why does CLinkedList inherit from CNode?

6) Your CLinkedList class violates the rule of 3 / rule of 2, which will cause code like below to crash:
{
  int value = 42;
  CLinkedList<int> myList;
  myList.Push( &value );
 
  CLinkedList<int> myOtherList = myList; // at the moment, this will perform a shallow copy
  //both lists now point to the same nodes!
  //when they're destructed, they will both try to delete these same nodes!
}//crash!!! double deletion bug!
You can fix this in two ways:
A) make CLinkedList non-copyable:
template <class DataT> class CLinkedList {	
private:
  CLinkedList( const CLinkedList& );
  CLinkedList& operator=( const CLinkedList& );
  //if anyone tries to copy a CLinkedList now, they'll get a compile-time error saying that copying is private
...
B) Implement copying
template <class DataT> class CLinkedList {
...	
public:
...
  CLinkedList( const CLinkedList& otherList )
  {
    First = NULL;
    Last  = NULL;
    NumNodesAllocated = 0;
    *this = otherList;
  }
  CLinkedList& operator=( const CLinkedList& otherList )
  {
    Clear();
    for each node in otherList //pseudo code
      this.Push(node.Data);
  }
...

#3Hodgman

Posted 15 September 2013 - 11:36 PM

Yes, all the std:: containers work well with any data type.

There are some issues with your container though:

1)
ZeroMemory(&Data, sizeof(DataT));
If DataT is a non-POD class (if it has constructors, or members that have constructors), then this is very, very bad. Just delete that line.
 
e.g. in the example below, std::vector is a non-POD class. It has constructors/destructors that perform careful memory management. On line #1, this constructor allocates 42 integers.
On line #2, this vector is overwritten with zeros... This is a really bad error. In the best case, maybe your program won't crash right away, and you've simply corrupted the object.
On line #3, we try to use the corrupted object, and this will likely crash.
std::vector<int> myVector(42); // #1
ZeroMemory(&myVector, sizeof(std::vector<int>)); // #2
myVector[12] = 12; // #3
 
2)
 Your other constructor isn't wrong, but it's very inefficient:
/// old
	CNode(DataT d)//copying 'd' by value here, means that the input class is copied from it's original location to 'd'
	{//hidden here: C++ default-constructs all members
		Data = d;//after 'Data' has been default-constructed, then copy d over the top of it
		Next = NULL;
		Prev = NULL;
	}
///new
	CNode(const DataT& d) // don't pass 'd' by value, pass by reference to avoid making an extra copy
	 : Data(d) // instead of default-constructing Data, use a copy-constructor
	{
		Next = NULL;
		Prev = NULL;
	}
 
3) You wrote that constructor above that takes a DataT argument, but don't even use it:
/// old
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(DataT *d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>;//default constructs 'Data', then uses ZeroMemory over it
	NumNodesAllocated++;

	pNewNode->Data = *d; // then copies '*d' over 'Data' again

/// new
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(const DataT& d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>(d)
	NumNodesAllocated++;

Other issues:

4) This has no place in a linked list class :-P
It has no impact on your question (whether classes work), but the average user would be surprised to find that the global srand value was being changed whenever they create a linked list.
srand(GetTickCount());
5) Why does CLinkedList inherit from CNode?

6) Your CLinkedList class violates the rule of 3 / rule of 2, which will cause code like below to crash:
{
  int value = 42;
  CLinkedList<int> myList;
  myList.Push( &value );
 
  CLinkedList<int> myOtherList = myList; // at the moment, this will perform a shallow copy
  //both lists now point to the same nodes!
  //when they're destructed, they will both try to delete these same nodes!
}//crash!!! double deletion bug!
You can fix this in two ways:
A) make CLinkedList non-copyable:
template <class DataT> class CLinkedList {	
private:
  CLinkedList( const CLinkedList& );
  CLinkedList& operator=( const CLinkedList& );
  //if anyone tries to copy a CLinkedList now, they'll get a compile-time error saying that copying is private
...
B) Implement copying
template <class DataT> class CLinkedList {
...	
public:
...
  CLinkedList( const CLinkedList& otherList )
  {
    First = NULL;
    Last  = NULL;
    NumNodesAllocated = 0;
    *this = otherList;
  }
  CLinkedList& operator=( const CLinkedList& otherList )
  {
    Clear();
    for each node in otherList //pseudo code
      this.Push(node.Data);
  }
...

#2Hodgman

Posted 15 September 2013 - 11:36 PM

Yes, all the std:: containers work well with any data type.

There are some issues with your container though:

1)
ZeroMemory(&Data, sizeof(DataT));
If DataT is a non-POD class (if it has constructors, or members that have constructors), then this is very, very bad. Just delete that line.
 
e.g. in the example below, std::vector is a non-POD class. It has constructors/destructors that perform careful memory management. On line #1, this constructor allocates 42 integers.
On line #2, this vector is overwritten with zeros... This is a really bad error. In the best case, maybe your program won't crash right away, and you've simply corrupted the object.
On line #3, we try to use the corrupted object, and this will likely crash.
std::vector<int> myVector(42); // #1
ZeroMemory(&myVector, sizeof(std::vector<int>)); // #2
myVector[12] = 12; // #3
 
2)
 Your other constructor isn't wrong, but it's very inefficient:
/// old
	CNode(DataT d)//copying 'd' by value here, means that the input class is copied from it's original location to 'd'
	{//hidden here: C++ default-constructs all members
		Data = d;//after 'Data' has been default-constructed, then copy d over the top of it
		Next = NULL;
		Prev = NULL;
	}
///new
	CNode(const DataT& d) // don't pass 'd' by value, pass by reference to avoid making an extra copy
	 : Data(d) // instead of default-constructing Data, use a copy-constructor
	{
		Next = NULL;
		Prev = NULL;
	}
 
3) You wrote that constructor above that takes a DataT argument, but don't even use it:
/// old
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(DataT *d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>;//default constructs 'Data', then uses ZeroMemory over it
	NumNodesAllocated++;

	pNewNode->Data = *d; // then copies '*d' over 'Data' again

/// new
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(const DataT& d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>(d)
	NumNodesAllocated++;
Other issues:

4) This has no place in a linked list class :-P
It has no impact on your question (whether classes work), but the average user would be surprised to find that the global srand value was being changed whenever they create a linked list.
srand(GetTickCount());
5) Why does CLinkedList inherit from CNode?

6) Your CLinkedList class violates the rule of 3 / rule of 2, which will cause code like below to crash:
{
  int value = 42;
  CLinkedList<int> myList;
  myList.Push( &value );
 
  CLinkedList<int> myOtherList = myList; // at the moment, this will perform a shallow copy
  //both lists now point to the same nodes!
  //when they're destructed, they will both try to delete these same nodes!
}//crash!!! double deletion bug!
You can fix this in two ways:
A) make CLinkedList non-copyable:
template <class DataT> class CLinkedList {	
private:
  CLinkedList( const CLinkedList& );
  CLinkedList& operator=( const CLinkedList& );
  //if anyone tries to copy a CLinkedList now, they'll get a compile-time error saying that copying is private
...
B) Implement copying
template <class DataT> class CLinkedList {
...	
public:
...
  CLinkedList( const CLinkedList& otherList )
  {
    First = NULL;
    Last  = NULL;
    NumNodesAllocated = 0;
    *this = otherList;
  }
  CLinkedList& operator=( const CLinkedList& otherList )
  {
    Clear();
    for each node in otherList //pseudo code
      this.Push(node.Data);
  }
...

#1Hodgman

Posted 15 September 2013 - 11:23 PM

1)
ZeroMemory(&Data, sizeof(DataT));
If DataT is a non-POD class (if it has constructors, or members that have constructors), then this is very, very bad. Just delete that line.
 
e.g. in the example below, std::vector is a non-POD class. It has constructors/destructors that perform careful memory management. On line #1, this constructor allocates 42 integers.
On line #2, this vector is overwritten with zeros... This is a really bad error. In the best case, maybe your program won't crash right away, and you've simply corrupted the object.
On line #3, we try to use the corrupted object, and this will likely crash.
std::vector<int> myVector(42); // #1
ZeroMemory(&myVector, sizeof(std::vector<int>)); // #2
myVector[12] = 12; // #3
 

2)
 Your other constructor isn't wrong, but it's very inefficient:
/// old
	CNode(DataT d)//copying 'd' by value here, means that the input class is copied from it's original location to 'd'
	{//hidden here: C++ default-constructs all members
		Data = d;//after 'Data' has been default-constructed, then copy d over the top of it
		Next = NULL;
		Prev = NULL;
	}
///new
	CNode(const DataT& d) // don't pass 'd' by value, pass by reference to avoid making an extra copy
	 : Data(d) // instead of default-constructing Data, use a copy-constructor
	{
		Next = NULL;
		Prev = NULL;
	}
3) This has no place in a linked list class :-P
srand(GetTickCount());

4) You wrote that constructor above that takes a DataT argument, but don't even use it:
/// old
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(DataT *d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>;//default constructs 'Data', then uses ZeroMemory over it
	NumNodesAllocated++;

	pNewNode->Data = *d; // the copies '*d' over 'Data' again

/// new
template <class DataT> CNode<DataT>* CLinkedList<DataT>::Push(const DataT& d)
{
	CNode<DataT> *pNewNode = new CNode<DataT>(d)
	NumNodesAllocated++;

PARTNERS