Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)

[Han-Wen Nienhuys]

On Wed, Aug 24, 2011 at 3:38 AM, David Kastrup <address@hidden> wrote:
> address@hidden writes:
>
>> Reviewers: ,
>>
>> Message:
>> This patch fixes a memleak: Some temporary skylines were never
>> deleted...
>> Please review
>>
>> Description:
>> Fix memleak: temporary skyline objects for systems were never deleted
>>
>> Please review this at http://codereview.appspot.com/4923048/
>>
>> Affected files:
>>    M lily/skyline.cc
>>
>>
>> Index: lily/skyline.cc
>> diff --git a/lily/skyline.cc b/lily/skyline.cc
>> index
>> b6ea6b791bbbfa09aaa28e91da0a619c33085890..8f62710b0947fa8e48ffcbc7e181cba4709c96de
>> 100644
>> --- a/lily/skyline.cc
>> +++ b/lily/skyline.cc
>> @@ -514,6 +514,7 @@ Skyline::distance (Skyline const &other, Real
>> horizon_padding) const
>>
>>     Skyline const *padded_this = this;
>>     Skyline const *padded_other = &other;
>> +  bool created_tmp_skylines = false;
>>
>>     /*
>>       For systems, padding is not added at creation time.  Padding is
>> @@ -525,6 +526,7 @@ Skyline::distance (Skyline const &other, Real
>> horizon_padding) const
>>       {
>>         padded_this = new Skyline (*padded_this, horizon_padding, X_AXIS);
>>         padded_other = new Skyline (*padded_other, horizon_padding, X_AXIS);
>> +      created_tmp_skylines = true;
>>       }
>>
>>     list<Building>::const_iterator i = padded_this->buildings_.begin ();
>> @@ -544,6 +546,13 @@ Skyline::distance (Skyline const &other, Real
>> horizon_padding) const
>>           j++;
>>         start = end;
>>       }
>> +
>> +  if (created_tmp_skylines)
>> +    {
>> +      delete padded_this;
>> +      delete padded_other;
>> +    }
>> +
>>     return dist;
>>   }
>
> Skylines are smobs.  The usual way to delete them would be to unprotect
> them once they have been registered by some garbage-collectable object
> (or a SCM variable that is being used for accessing them).

They are simple smobs, though, so this pattern here is not uncommon.
You could also try to allocate them on the stack.

--
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen