map suddenly contains null pointers

[acgrama]

Hello all,

I have a class DeviceManager in which I have a global variable as follows:

...
map<long, Device *> devices; // global variable
...
// Definition of DeviceManager functions...
...
void DeviceManager::function1(){...}
void DeviceManager::function2(){...}
...

In function1(), I add in the map some (long, Device *) pairs. However, sometimes when I try to access the devices map in function2(), the pointers in the map are all NULL!

I hope I am making some huge newbie mistake -- does anyone have an idea why this might happen?
Thank you in advance!
--Cristina.

[firedraco]

Can you show the relevant code in function1/2?

Right now I have no idea why it would do that, except maybe you are accessing pairs you didn't add.

[Incubbus]

or the devices are released...

[acgrama]

Power of posting, I think I managed to find out what the problem was :)

In function1() I was filling up the map like this:

ChannelDevice *device; // ChannelDevice is a class which extends Device

for (....){
device = new ChannelDevice();
device->a = ..
device->b = ..

devices[index] = device;

delete device;// <----- This is where the error occurred :D Brought about by my C++ newbie state
}

Did not realise I was deleting the pointer and not just freeing up the device variable to be used in the next cycle of the for loop.

And a related question: is what I did in the for loop "best practice" for initialising a structure with pointers?
Also: thank you for the fast reaction to my initial post ;-)

[sammy34]

I'm no expert, but to my knowledge one of the main uses of a constructor is to initialise relevant variables.

In your for loop above, you're creating a new ChannelDevice object using the default constructor for that class, and then setting the variables separately afterwards:

device = new ChannelDevice();
device->a = ..
device->b = ..

I think the 'best practice' would be to pass these values into a user-defined constructor that you write yourself inside the class. For example:

ChannelDevice(TypeA initA, TypeB initB)
{
//Initialisation code
a = initA;
b = initB;
...
}

I'm not sure what 'a' and 'b' represent, but perhaps it's not appropriate to give them public visibility. If you make them private inside the class and initialise them using such a constructor, encapsulation in the OO sense is promoted.

Apart from that, I don't see anything wrong with the way you're populating 'devices'.

[acgrama]

Thank you for your advice! :)

[Disch]

You have a problem here:

device = new ChannelDevice();   // create a device
//...
devices[index] = device;  // copy a pointer tothat device to your map

delete device;// delete the device 

Note that you are DELETING the device. This means the device no longer exists. This means the pointer you put in your map points to a device that no longer exists (ie: bad pointer)

Either:

- Put device objects in your map (not pointers)
or
- don't delete the device until you're not using it anymore (ie: your map isn't using it either)

[jsmith]

sammy34's advice is good, but it is better to use initializer lists in constructors:

ChannelDevice(TypeA initA, TypeB initB) : a( initA ), b( initB )
{
}

[sammy34]

Fair call j, I agree that initializer lists are better.

Once you've put in place jsmith's constructor, your 'for' loop can be condensed to:

for (....)
{
devices[index] = new ChannelDevice(a, b);
}

I think that looks much nicer. You can always put a detailed comment in the loop if you believe that it's hard to understand.

As Disch said, you will of course have to release the memory (dynamically allocated by the 'new' operator in each case) after you've finished with your Devices...