Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make DcmList random access be O(1) #113

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 9 additions & 57 deletions dcmdata/include/dcmtk/dcmdata/dclist.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,53 +22,13 @@
#ifndef DCLIST_H
#define DCLIST_H

#include "dcmtk/config/osconfig.h" /* make sure OS specific configuration is included first */

#include "dcmtk/ofstd/ofcast.h"
#include "dcmtk/ofstd/oftypes.h"

#include "dcmtk/dcmdata/dcobject.h"

#include <deque>

/// index indicating "end of list"
const unsigned long DCM_EndOfListIndex = OFstatic_cast(unsigned long, -1L);

/** helper class maintaining an entry in a DcmList double-linked list
*/
class DCMTK_DCMDATA_EXPORT DcmListNode
{

public:
/** constructor
* @param obj object to be maintained by this list node
*/
DcmListNode( DcmObject *obj );

/// destructor
~DcmListNode();

/// return pointer to object maintained by this list node
inline DcmObject *value() { return objNodeValue; }

private:
friend class DcmList;

/// pointer to next node in double-linked list
DcmListNode *nextNode;

/// pointer to previous node in double-linked list
DcmListNode *prevNode;

/// pointer to DcmObject instance maintained by this list entry
DcmObject *objNodeValue;

/// private undefined copy constructor
DcmListNode(const DcmListNode &);

/// private undefined copy assignment operator
DcmListNode &operator=(const DcmListNode &);

};

/// list position indicator
typedef enum
{
Expand Down Expand Up @@ -101,13 +61,13 @@ class DCMTK_DCMDATA_EXPORT DcmList
/// destructor
~DcmList();

/** insert object at end of list
/** insert object at end of list and make it be the "current" one
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the existing behavior for clarity (might be relevant on the call site).

* @param obj pointer to object
* @return pointer to object
*/
DcmObject *append( DcmObject *obj );

/** insert object at start of list
/** insert object at start of list and make it be the "current" one
* @param obj pointer to object
* @return pointer to object
*/
Expand Down Expand Up @@ -153,27 +113,19 @@ class DCMTK_DCMDATA_EXPORT DcmList
void deleteAllElements();

/// return cardinality of list
inline unsigned long card() const { return cardinality; }
inline unsigned long card() const { return static_cast<unsigned long>(objects.size()); }

/// return true if list is empty, false otherwise
inline OFBool empty(void) const { return firstNode == NULL; }
inline OFBool empty(void) const { return objects.empty(); }

/// return true if current node exists, false otherwise
inline OFBool valid(void) const { return currentNode != NULL; }
inline OFBool valid(void) const { return current != objects.end(); }

private:
/// pointer to first node in list
DcmListNode *firstNode;

/// pointer to last node in list
DcmListNode *lastNode;
std::deque<DcmObject*> objects;

/// pointer to current node in list
DcmListNode *currentNode;
std::deque<DcmObject*>::iterator current;

/// number of elements in list
unsigned long cardinality;

/// private undefined copy constructor
DcmList &operator=(const DcmList &);

Expand Down
180 changes: 29 additions & 151 deletions dcmdata/libsrc/dclist.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,43 +19,16 @@
*
*/

#include "dcmtk/config/osconfig.h" /* make sure OS specific configuration is included first */

#include "dcmtk/ofstd/ofstream.h"
#include "dcmtk/dcmdata/dclist.h"


// *****************************************
// *** DcmListNode *************************
// *****************************************


DcmListNode::DcmListNode( DcmObject *obj )
: nextNode(NULL),
prevNode(NULL),
objNodeValue(obj)
{
}


// ********************************


DcmListNode::~DcmListNode()
{
}


// *****************************************
// *** DcmList *****************************
// *****************************************


DcmList::DcmList()
: firstNode(NULL),
lastNode(NULL),
currentNode(NULL),
cardinality(0)
: current(objects.end())
{
}

Expand All @@ -65,17 +38,7 @@ DcmList::DcmList()

DcmList::~DcmList()
{
if ( !DcmList::empty() ) // list is not empty !
{
lastNode->nextNode = NULL; // set to 0 for safety reasons
do {
DcmListNode *temp = firstNode;
firstNode = firstNode->nextNode;
// delete temp->objNodeValue; // dangerous!
delete temp;
} while ( firstNode != NULL );
currentNode = firstNode = lastNode = NULL;
}
deleteAllElements();
}


Expand All @@ -84,19 +47,7 @@ DcmList::~DcmList()

DcmObject *DcmList::append( DcmObject *obj )
{
if ( obj != NULL )
{
if ( DcmList::empty() ) // list is empty !
currentNode = firstNode = lastNode = new DcmListNode(obj);
else
{
DcmListNode *node = new DcmListNode(obj);
lastNode->nextNode = node;
node->prevNode = lastNode;
currentNode = lastNode = node;
}
cardinality++;
} // obj == NULL
current = objects.insert(objects.end(), obj);
return obj;
}

Expand All @@ -106,19 +57,7 @@ DcmObject *DcmList::append( DcmObject *obj )

DcmObject *DcmList::prepend( DcmObject *obj )
{
if ( obj != NULL )
{
if ( DcmList::empty() ) // list is empty !
currentNode = firstNode = lastNode = new DcmListNode(obj);
else
{
DcmListNode *node = new DcmListNode(obj);
node->nextNode = firstNode;
firstNode->prevNode = node;
currentNode = firstNode = node;
}
cardinality++;
} // obj == NULL
current = objects.insert(objects.begin(), obj);
return obj;
}

Expand All @@ -132,8 +71,7 @@ DcmObject *DcmList::insert( DcmObject *obj, E_ListPos pos )
{
if ( DcmList::empty() ) // list is empty !
{
currentNode = firstNode = lastNode = new DcmListNode(obj);
cardinality++;
DcmList::append(obj);
}
else {
if ( pos==ELP_last )
Expand All @@ -146,30 +84,12 @@ DcmObject *DcmList::insert( DcmObject *obj, E_ListPos pos )
DcmList::append( obj ); // cardinality++;
else if ( pos == ELP_prev ) // insert before current node
{
DcmListNode *node = new DcmListNode(obj);
if ( currentNode->prevNode == NULL )
firstNode = node; // insert at the beginning
else
currentNode->prevNode->nextNode = node;
node->prevNode = currentNode->prevNode;
node->nextNode = currentNode;
currentNode->prevNode = node;
currentNode = node;
cardinality++;
current = objects.insert(current, obj);
}
else //( pos==ELP_next || pos==ELP_atpos )
// insert after current node
{
DcmListNode *node = new DcmListNode(obj);
if ( currentNode->nextNode == NULL )
lastNode = node; // append to the end
else
currentNode->nextNode->prevNode = node;
node->nextNode = currentNode->nextNode;
node->prevNode = currentNode;
currentNode->nextNode = node;
currentNode = node;
cardinality++;
current = objects.insert(std::next(current), obj);
}
}
} // obj == NULL
Expand All @@ -183,30 +103,15 @@ DcmObject *DcmList::insert( DcmObject *obj, E_ListPos pos )
DcmObject *DcmList::remove()
{
DcmObject *tempobj;
DcmListNode *tempnode;

if ( DcmList::empty() ) // list is empty !
return NULL;
else if ( !DcmList::valid() )
return NULL; // current node is 0
else
{
tempnode = currentNode;

if ( currentNode->prevNode == NULL )
firstNode = currentNode->nextNode; // delete first element
else
currentNode->prevNode->nextNode = currentNode->nextNode;

if ( currentNode->nextNode == NULL )
lastNode = currentNode->prevNode; // delete last element
else
currentNode->nextNode->prevNode = currentNode->prevNode;

currentNode = currentNode->nextNode;
tempobj = tempnode->value();
delete tempnode;
cardinality--;
tempobj = *current;
current = objects.erase(current);
return tempobj;
}
}
Expand All @@ -229,23 +134,29 @@ DcmObject *DcmList::seek( E_ListPos pos )
switch (pos)
{
case ELP_first :
currentNode = firstNode;
current = objects.begin();
break;
case ELP_last :
currentNode = lastNode;
if (objects.empty())
current = objects.end();
else
current = std::prev(objects.end());
break;
case ELP_prev :
if ( DcmList::valid() )
currentNode = currentNode->prevNode;
if (current == objects.begin())
current = objects.end();
else
--current;
break;
case ELP_next :
if ( DcmList::valid() )
currentNode = currentNode->nextNode;
++current;
break;
default:
break;
}
return DcmList::valid() ? currentNode->value() : NULL;
return DcmList::valid() ? *current : NULL;
}


Expand All @@ -254,23 +165,11 @@ DcmObject *DcmList::seek( E_ListPos pos )

DcmObject *DcmList::seek_to(unsigned long absolute_position)
{
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this O(n) search to an O(1) lookup is the beef of this PR.

if (absolute_position < cardinality / 2)
{
/* iterate over first half of the list */
seek( ELP_first );
for (unsigned long i = 0; i < absolute_position; i++)
seek( ELP_next );
}
else if (absolute_position < cardinality)
{
/* iterate over second half of the list (starting from the end) */
seek( ELP_last );
for (unsigned long i = absolute_position + 1; i < cardinality; i++)
seek( ELP_prev );
} else {
/* invalid position */
currentNode = NULL;
}
if (absolute_position < objects.size())
current = objects.begin() + absolute_position;
else
current = objects.end();

return get( ELP_atpos );
}

Expand All @@ -280,29 +179,8 @@ DcmObject *DcmList::seek_to(unsigned long absolute_position)

void DcmList::deleteAllElements()
{
unsigned long numElements = cardinality;
DcmListNode* tmpNode = NULL;
DcmObject* tmpObject = NULL;
// delete all elements
for (unsigned long i = 0; i < numElements; i++)
{
// always select first node so no search is necessary
tmpNode = firstNode;
// clear value of node
tmpObject = tmpNode->value();
if (tmpObject != NULL)
{
// delete load of selected list node
delete tmpObject;
tmpObject = NULL;
}
firstNode = tmpNode->nextNode;
// delete the list node itself
delete tmpNode;
}
// reset all attributes for later use
firstNode = NULL;
lastNode = NULL;
currentNode = NULL;
cardinality = 0;
for (DcmObject* object : objects)
delete object;
objects.clear();
current = objects.end();
}