On Tue, Jan 19, 2010 at 9:45 AM, Ted Kremenek <[email protected]> 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 <[email protected]> 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
