• 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;
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:
template <class DataT> class CLinkedList {
private:
//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:
...
{
First = NULL;
Last  = NULL;
NumNodesAllocated = 0;
*this = 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;
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:
template <class DataT> class CLinkedList {
private:
//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:
...
{
First = NULL;
Last  = NULL;
NumNodesAllocated = 0;
*this = 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;
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:
template <class DataT> class CLinkedList {
private:
//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:
...
{
First = NULL;
Last  = NULL;
NumNodesAllocated = 0;
*this = 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;
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:
template <class DataT> class CLinkedList {
private:
//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:
...
{
First = NULL;
Last  = NULL;
NumNodesAllocated = 0;
*this = 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