Jump to content

  • Log In with Google      Sign In   
  • Create Account


#ActualDissipate

Posted 28 January 2013 - 06:28 PM

Posted 24 January 2013 - 05:49 PM

DeleteAllNodes definitely contains a bug, it fails to clear the "head" and "tail" pointers. CNode calls "delete" on a VOID pointer, which is undefined. You should use templates to maintain type safety. The destructor does not deallocate memory.
The lack of copy constructors and assignment operators on the two classes is also problematic. You need to obey the rule of three, or "disable" copying and assignment by declaring the copy constructor and assignment operator as private, and not implementing them.
I don't see any other major bugs, but due to the unencapsulated implementation every single client access needs to be perfectly correct or it could corrupt the loop.
There are a few minor issues too:
(m_uNodeType != CNODE_HEAD) || (m_uNodeType != CNODE_END) is a tautology. Consider, if X is any number, then (X != 1) || (x != 2) is true.
You don't need to check a pointer is NULL before calling delete - delete does the right thing with null pointers and you are duplicating those checks.
You don't need to have two value nodes in the linked list. Most implementations just have a pair of pointers to nodes. Using such "sentinel" nodes isn't necessarily a bad thing, it is a valid implementation strategy, but you should not force client code to reason about this.
You don't need the "node type" concept. Just have the head and tail contain NULL data pointers.
Don't depend on Windows.h: just use void * rather than VOID * if it was necessary
Use an enum rather than #defines for something like "node type", if it was necessary
Main.h is almost always a poor idea.
In general, strive for as few dependencies as possible, particularly for something generic like a linked list. Avoid coupling this implementation to the rest of your project



I have made all the changes you suggested, making a Circular, templated, doubley linked list and applying the rule of three. Implementing this into my project and utilising it does not solve the problem (I'll research dynamic arrays later!). Please can you help me understand whats happening as I can't continue otherwise. Maybe someone can post a short excerpt/code-list on how to clone and draw a textured quad. My D3DXSprite clones fine.
(hoping...!)

#2Dissipate

Posted 28 January 2013 - 06:26 PM

Posted 24 January 2013 - 05:49 PM

DeleteAllNodes definitely contains a bug, it fails to clear the "head" and "tail" pointers. CNode calls "delete" on a VOID pointer, which is undefined. You should use templates to maintain type safety. The destructor does not deallocate memory.
The lack of copy constructors and assignment operators on the two classes is also problematic. You need to obey the rule of three, or "disable" copying and assignment by declaring the copy constructor and assignment operator as private, and not implementing them.
I don't see any other major bugs, but due to the unencapsulated implementation every single client access needs to be perfectly correct or it could corrupt the loop.
There are a few minor issues too:
(m_uNodeType != CNODE_HEAD) || (m_uNodeType != CNODE_END) is a tautology. Consider, if X is any number, then (X != 1) || (x != 2) is true.
You don't need to check a pointer is NULL before calling delete - delete does the right thing with null pointers and you are duplicating those checks.
You don't need to have two value nodes in the linked list. Most implementations just have a pair of pointers to nodes. Using such "sentinel" nodes isn't necessarily a bad thing, it is a valid implementation strategy, but you should not force client code to reason about this.
You don't need the "node type" concept. Just have the head and tail contain NULL data pointers.
Don't depend on Windows.h: just use void * rather than VOID * if it was necessary
Use an enum rather than #defines for something like "node type", if it was necessary
Main.h is almost always a poor idea.
In general, strive for as few dependencies as possible, particularly for something generic like a linked list. Avoid coupling this implementation to the rest of your project

 

I have made all the changes you suggested, making a Circular, templated, doubley linked list and applying the rule of three. Implementing this into my project and utilising it does not solve the problem (I'll research dynamic arrays later!). Please can you help me understand whats happening as I can't continue otherwise. Maybe someone can post a short excerpt/code-list on how to clone and draw a textured quad. My D3DXSprite clones fine.
(hoping...!)


#1Dissipate

Posted 28 January 2013 - 06:25 PM

Posted 24 January 2013 - 05:49 PM

DeleteAllNodes definitely contains a bug, it fails to clear the "head" and "tail" pointers. CNode calls "delete" on a VOID pointer, which is undefined. You should use templates to maintain type safety. The destructor does not deallocate memory.

The lack of copy constructors and assignment operators on the two classes is also problematic. You need to obey the rule of three, or "disable" copying and assignment by declaring the copy constructor and assignment operator as private, and not implementing them.

I don't see any other major bugs, but due to the unencapsulated implementation every single client access needs to be perfectly correct or it could corrupt the loop.

There are a few minor issues too:

(m_uNodeType != CNODE_HEAD) || (m_uNodeType != CNODE_END) is a tautology. Consider, if X is any number, then (X != 1) || (x != 2) is true.

You don't need to check a pointer is NULL before calling delete - delete does the right thing with null pointers and you are duplicating those checks.

You don't need to have two value nodes in the linked list. Most implementations just have a pair of pointers to nodes. Using such "sentinel" nodes isn't necessarily a bad thing, it is a valid implementation strategy, but you should not force client code to reason about this.

You don't need the "node type" concept. Just have the head and tail contain NULL data pointers.

Don't depend on Windows.h: just use void * rather than VOID * if it was necessary

Use an enum rather than #defines for something like "node type", if it was necessary

Main.h is almost always a poor idea.

In general, strive for as few dependencies as possible, particularly for something generic like a linked list. Avoid coupling this implementation to the rest of your project

 

I have made all the changes you suggested, making a Circular, templated, doubley linked list and applying the rule of three. Implementing this into my project and utilising it does not solve the problem (I'll research dynamic arrays later!). Please can you help me understand whats happening as I can't continue otherwise. Maybe someone can post a short excerpt/code-list on how to clone and draw a textured quad. My D3DXSprite clones fine. 

(hoping...!)


PARTNERS