[patch] slightly controversial refactor

Jeff Mitchell kde-dev at emailgoeshere.com
Sat Jan 20 09:27:04 UTC 2007


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



More information about the Amarok mailing list