I need to clean up some old code which implements a static instance of
a class.
Please comment out whether my clean up is good.
My goal is to change the old code as little as possible.
Old code are:
Appliance.h
class Appliance {
//...
private:
static FastMap* ReaderSerialNumberMapping;
};

Appliance.cpp
FastMap* Appliance::ReaderSerialNumberMapping=NULL;


Appliance::Appliance(unsigned int serialNumber) {
if( ReaderSerialNumberMapping==NULL )
{
ReaderSerialNumberMapping = new FastMap(16);
}
//...
ReaderSerialNumberMapping->Assign( serialNumber, readerNumber );
//...
}

void Appliance::doY() {
//...
readerNumber = ReaderSerialNumberMapping->get( serialNumber);
//...
}

// The old code does not have a destructor so it does not delete the
memory pointed by the ReaderSerialNumberMapping. Therefore I want to
fix the memory leak problem by changing the implementation of
ReaderSerialNumberMapping to not using the pointer.
// New code are as follows:
Appliance.h
class Appliance {
//...
private:
static FastMap ReaderSerialNumberMapping;
}

Appliance.cpp
FastMap Appliance::ReaderSerialNumberMapping = FastMap(16);

void Appliance::Appliance(unsigned int serialNumber) {
//...
ReaderSerialNumberMapping.Assign( serialNumber, readerNumber );
}

void Appliance::doY() {
//...
readerNumber = ReaderSerialNumberMapping.get( serialNumber);
//...
}

wong_powah@yahoo.ca napsal:

I need to clean up some old code which implements a static instance of
a class.
Please comment out whether my clean up is good.
My goal is to change the old code as little as possible.
Old code are:
Appliance.h
class Appliance {
//...
private:
static FastMap* ReaderSerialNumberMapping;
};
>
Appliance.cpp
FastMap* Appliance::ReaderSerialNumberMapping=NULL;
>
>
Appliance::Appliance(unsigned int serialNumber) {
if( ReaderSerialNumberMapping==NULL )
{
ReaderSerialNumberMapping = new FastMap(16);
}
//...
ReaderSerialNumberMapping->Assign( serialNumber, readerNumber );
//...
}
>
void Appliance::doY() {
//...
readerNumber = ReaderSerialNumberMapping->get( serialNumber);
//...
}
>
// The old code does not have a destructor so it does not delete the
memory pointed by the ReaderSerialNumberMapping. Therefore I want to
fix the memory leak problem by changing the implementation of
ReaderSerialNumberMapping to not using the pointer.
// New code are as follows:
Appliance.h
class Appliance {
//...
private:
static FastMap ReaderSerialNumberMapping;
}
>
Appliance.cpp
FastMap Appliance::ReaderSerialNumberMapping = FastMap(16);
>
void Appliance::Appliance(unsigned int serialNumber) {
//...
ReaderSerialNumberMapping.Assign( serialNumber, readerNumber );
}
>
void Appliance::doY() {
//...
readerNumber = ReaderSerialNumberMapping.get( serialNumber);
//...
}

I think it's ok. In case you need static variable as pointer, you can
use function atexit to destroy it or wrap it inside the std::auto_ptr.

wong_powah@yahoo.ca wrote:

I need to clean up some old code which implements a static instance of
a class.
Please comment out whether my clean up is good.
My goal is to change the old code as little as possible.
Old code are:
Appliance.h
class Appliance {
//...
private:
static FastMap* ReaderSerialNumberMapping;
};
>
Appliance.cpp
FastMap* Appliance::ReaderSerialNumberMapping=NULL;
>
>
Appliance::Appliance(unsigned int serialNumber) {
if( ReaderSerialNumberMapping==NULL )
{
ReaderSerialNumberMapping = new FastMap(16);
}
//...
ReaderSerialNumberMapping->Assign( serialNumber, readerNumber );
//...
}
>
void Appliance::doY() {
//...
readerNumber = ReaderSerialNumberMapping->get( serialNumber);
//...
}
>
// The old code does not have a destructor so it does not delete the
memory pointed by the ReaderSerialNumberMapping. Therefore I want to
fix the memory leak problem by changing the implementation of
ReaderSerialNumberMapping to not using the pointer.
// New code are as follows:
Appliance.h
class Appliance {
//...
private:
static FastMap ReaderSerialNumberMapping;
}
>
Appliance.cpp
FastMap Appliance::ReaderSerialNumberMapping = FastMap(16);
>
void Appliance::Appliance(unsigned int serialNumber) {
//...
ReaderSerialNumberMapping.Assign( serialNumber, readerNumber );
}
>
void Appliance::doY() {
//...
readerNumber = ReaderSerialNumberMapping.get( serialNumber);
//...
}

The original implementation had a life-time functionality that your's
doesn't. if you want to limit the keyboard time it would be simplest
to change the ReaderSerialNumberMapping to auto_ptr and carry on from
there. Otherwise it seems to be equivalent.

dasjotre wrote:

The original implementation had a life-time functionality that your's
doesn't.

What is your meaning of "life-time functionality"?
My implementation use the "class static data member" mechanism. It
acts as a global object that belongs to its class type. Its life time
is the same as a global object. However, it is not entered into the
program's global namespace.

wong_powah@yahoo.ca wrote:

dasjotre wrote:

>
What is your meaning of "life-time functionality"?
My implementation use the "class static data member" mechanism. It
acts as a global object that belongs to its class type. Its life time
is the same as a global object. However, it is not entered into the
program's global namespace.

The original didn't create the pointed-at FastMap till the first time
the containing class got an instance created. Your implementation
creates it earlier because it is a static. That's not necessarily a
problem, just that you should be aware of the time-of-creation
issue that is going on here. For example, there might be side
effects of creating the FastMap, and these might be important.
Or, it might be that your program could possibly run without
ever actually creating an instance of the containing class, and
so you might not actually ever need the FastMap. Again, all
that might not matter, you should just be aware of it in case it
actually does matter.
Socks