[patch] slightly controversial refactor

[Andrew Turner]

Here's a third opinion for you:

Obviously, the pointer itself is deleted when the pointer goes out of
scope, just like any other stack-allocated object. However, the object
it points to is not stack-allocated and without KDE's cool
parent-based widget deletion, if you forget about an object allocated
through "new", it just stays sitting there as a memory leak.

The only problem I can see with the code (pre-625102) is that while I
assume the m_mediumMap should hold the pointers to all the allocated
objects, you have the situation (when the named medium is found) of
allocating one object for returnedMedium, but another for m_mediumMap.
This is presumably because the objects in m_mediumMap can be deleted
at any time (the same function seems to like deleting and recreating
them whenever it's run), however this design does place the burden on
the caller to delete the returned object when they're done with it.
I'm not quite sure though, if the m_mediumMap contents can't be
trusted (because we might delete the objects pointed to), what their
purpose is.

One thing worth noting is that if you call getDevice with a name like
"init", which it is assumed no device will have (presumably), then
there will never be another allocation for returnedMedium, because it
won't be found. Therefore, no memory leak in that case (but it does no
harm to be safe and delete the null pointer).

Also, I don't truly know whether all the m_mediumMap objects get freed
eventually. Certainly there seems to be code to free them in
mediumRemoved, but I would have expected a destructor for the
DeviceManager to free them too. However, just because there isn't one,
doesn't mean it doesn't get done somewhere.

As for Eric's patches, the first one (already applied) doesn't appear
to actually fix any leaks, but it won't hurt. The second one does, I
think, improve clarity and is worth applying. I don't understand why
deleting and then recreating every device is necessary, though, but
the old code did it too.

Andrew

On 20/01/07, Jeff Mitchell <kde-dev at emailgoeshere.com> wrote:
> Erik--
>
> The patch looks fine.  You were right that the function was being run those
> two times just to get the map populated when it needs to be.
>
> The reasons why that is a leak are a bit unclear to me.  Looking at the
> pre-625102 code, in getDevice a pointer is allocated on the stack and the
> value is returned.  The return value is ignored (nothing is assigned to it).
> But the pointer used to return the value was allocated locally inside
> getDevice on the stack, not on the heap.  So I had thought that like, say, a
> local stack-based int or double, that pointer goes out of scope when
> getDevice exits, and memory-wise things were fine.  Can you explain why this
> is not so?
>
> Thanks,
> Jeff
>
> On Friday 19 January 2007 09:19, Erik Hovland wrote:
> > I know I have svn write access so posting here is really for my own warm
> > fuzzies.
> >
> > Some of you might have noticed commit version 625102 where I closed a
> > couple of small memory leaks. When closing those leaks I noticed that
> > the two callers of DeviceManager::getDevice( name ) were not calling
> > them to get a device. And that they had leaks specifically because they
> > could care less if they got a device. Instead they meant to query kded
> > for any devices and put them in the device manager. This is evident when
> > you notice that DeviceManager::getDevice( name ) is coded up so that it
> > intentially has a side effect of reconciling the
> > DeviceManager::m_mediumMap.
> >
> > So I decided to do a little refactoring. First, I recoded getDevice() so
> > that it does get the device and only the device (reconciling only that
> > device in the m_mediumMap). Then I wrote another member function called
> > reconcileMediumMap() to do the work of reconciling m_mediumMap to what
> > ever kded returns. Now switch the two errant callers to the new
> > function... and viola! Now we have consistent use of DeviceManager.
> >
> > I even tried this patch out. But I am no device/mount point expert. So
> > rather then commit my junk and live with it, I figured I would at least
> > submit it to the list for the once over. Let me know what you think. I
> > am willing to rework it or drop it since my previous fix does close the
> > leak.
> >
> > Thanks
> >
> > E
> _______________________________________________
> Amarok mailing list
> Amarok at kde.org
> https://mail.kde.org/mailman/listinfo/amarok
>