[cfe-commits] r93834 - /cfe/trunk/lib/Driver/Driver.cpp

[Daniel Dunbar]

On Tue, Jan 19, 2010 at 9:45 AM, Ted Kremenek <kremenek at apple.com> wrote:
> I've looked at this again, and you're right there isn't a leak.  It wasn't clear to me what the ownership model was, but now I see that what was there before was fine.  I personally prefer it with the OwningPtr, as it makes the memory management very clear, but it's up to you.

I actually dislike using OwningPtr like this. The problem is using
OwningPtr makes it "look safe", even though the code is still
essentially managing the memory by hand (and relying on other
invariants for ownership). If its going to do that, I prefer the alarm
bells to ring.

 - Daniel

> On Jan 19, 2010, at 9:02 AM, Daniel Dunbar wrote:
>
>> Hi Ted,
>>
>> On Mon, Jan 18, 2010 at 5:29 PM, Ted Kremenek <kremenek at apple.com> wrote:
>>> Author: kremenek
>>> Date: Mon Jan 18 19:29:05 2010
>>> New Revision: 93834
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=93834&view=rev
>>> Log:
>>> Fix possible memory leak by using an OwningPtr.
>>
>> I can't see what the possible leak was, can you explain?
>>
>> - Daniel
>>
>>> Modified:
>>>    cfe/trunk/lib/Driver/Driver.cpp
>>>
>>> Modified: cfe/trunk/lib/Driver/Driver.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=93834&r1=93833&r2=93834&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Driver/Driver.cpp (original)
>>> +++ cfe/trunk/lib/Driver/Driver.cpp Mon Jan 18 19:29:05 2010
>>> @@ -26,6 +26,7 @@
>>>  #include "clang/Basic/Version.h"
>>>
>>>  #include "llvm/ADT/StringSet.h"
>>> +#include "llvm/ADT/OwningPtr.h"
>>>  #include "llvm/Support/PrettyStackTrace.h"
>>>  #include "llvm/Support/raw_ostream.h"
>>>  #include "llvm/System/Path.h"
>>> @@ -675,7 +676,7 @@
>>>     }
>>>
>>>     // Build the pipeline for this file.
>>> -    Action *Current = new InputAction(*InputArg, InputType);
>>> +    llvm::OwningPtr<Action> Current(new InputAction(*InputArg, InputType));
>>>     for (unsigned i = 0; i != NumSteps; ++i) {
>>>       phases::ID Phase = types::getCompilationPhase(InputType, i);
>>>
>>> @@ -686,8 +687,7 @@
>>>       // Queue linker inputs.
>>>       if (Phase == phases::Link) {
>>>         assert(i + 1 == NumSteps && "linking must be final compilation step.");
>>> -        LinkerInputs.push_back(Current);
>>> -        Current = 0;
>>> +        LinkerInputs.push_back(Current.take());
>>>         break;
>>>       }
>>>
>>> @@ -698,14 +698,14 @@
>>>         continue;
>>>
>>>       // Otherwise construct the appropriate action.
>>> -      Current = ConstructPhaseAction(Args, Phase, Current);
>>> +      Current.reset(ConstructPhaseAction(Args, Phase, Current.take()));
>>>       if (Current->getType() == types::TY_Nothing)
>>>         break;
>>>     }
>>>
>>>     // If we ended with something, add to the output list.
>>>     if (Current)
>>> -      Actions.push_back(Current);
>>> +      Actions.push_back(Current.take());
>>>   }
>>>
>>>   // Add a link action if necessary.
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>
>