[polly] r239061 - Use owning pointers to avoid memory leaks

[David Blaikie]

On Thu, Jun 4, 2015 at 10:59 AM, Tobias Grosser <tobias at grosser.es> wrote:

> Author: grosser
> Date: Thu Jun  4 12:59:54 2015
> New Revision: 239061
>
> URL: http://llvm.org/viewvc/llvm-project?rev=239061&view=rev
> Log:
> Use owning pointers to avoid memory leaks
>
> This fixes a memory leak caused by us not freeing the expanded region
> nodes.
>
> Modified:
>     polly/trunk/lib/Analysis/ScopDetection.cpp
>
> Modified: polly/trunk/lib/Analysis/ScopDetection.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopDetection.cpp?rev=239061&r1=239060&r2=239061&view=diff
>
> ==============================================================================
> --- polly/trunk/lib/Analysis/ScopDetection.cpp (original)
> +++ polly/trunk/lib/Analysis/ScopDetection.cpp Thu Jun  4 12:59:54 2015
> @@ -722,15 +722,15 @@ bool ScopDetection::isValidLoop(Loop *L,
>
>  Region *ScopDetection::expandRegion(Region &R) {
>    // Initial no valid region was found (greater than R)
> -  Region *LastValidRegion = nullptr;
> -  Region *ExpandedRegion = R.getExpandedRegion();
> +  std::unique_ptr<Region> LastValidRegion;
> +  auto ExpandedRegion = std::unique_ptr<Region>(R.getExpandedRegion());
>

Code like this scares me a bit - if R used to own the expanded region, how
does it track not owning it anymore?

(& if you prefer to write "std::unique_ptr<Region>
ExpandedRegion(R.getExpandedRegion())" I'm OK with that - I know Herb
Sutter espoused the "always use auto" idea, but it's not universally
accepted as the way to go)


>
>    DEBUG(dbgs() << "\tExpanding " << R.getNameStr() << "\n");
>
>    while (ExpandedRegion) {
>      DetectionContext Context(
> -        *ExpandedRegion, *AA, NonAffineSubRegionMap[ExpandedRegion],
> -        BoxedLoopsMap[ExpandedRegion], false /* verifying */);
> +        *ExpandedRegion, *AA, NonAffineSubRegionMap[ExpandedRegion.get()],
> +        BoxedLoopsMap[ExpandedRegion.get()], false /* verifying */);
>      DEBUG(dbgs() << "\t\tTrying " << ExpandedRegion->getNameStr() <<
> "\n");
>      // Only expand when we did not collect errors.
>
> @@ -742,25 +742,18 @@ Region *ScopDetection::expandRegion(Regi
>        if (!allBlocksValid(Context) || Context.Log.hasErrors())
>          break;
>
> -      // Delete unnecessary regions (allocated by getExpandedRegion)
> -      if (LastValidRegion)
> -        delete LastValidRegion;
> -
>        // Store this region, because it is the greatest valid (encountered
> so
>        // far).
> -      LastValidRegion = ExpandedRegion;
> +      LastValidRegion = std::move(ExpandedRegion);
>
>        // Create and test the next greater region (if any)
> -      ExpandedRegion = ExpandedRegion->getExpandedRegion();
> +      ExpandedRegion =
> +          std::unique_ptr<Region>(LastValidRegion->getExpandedRegion());
>
>      } else {
>        // Create and test the next greater region (if any)
> -      Region *TmpRegion = ExpandedRegion->getExpandedRegion();
> -
> -      // Delete unnecessary regions (allocated by getExpandedRegion)
> -      delete ExpandedRegion;
> -
> -      ExpandedRegion = TmpRegion;
> +      ExpandedRegion =
> +          std::unique_ptr<Region>(ExpandedRegion->getExpandedRegion());
>      }
>    }
>
> @@ -771,7 +764,7 @@ Region *ScopDetection::expandRegion(Regi
>        dbgs() << "\tExpanding " << R.getNameStr() << " failed\n";
>    });
>
> -  return LastValidRegion;
> +  return LastValidRegion.release();
>

Should this function return unique_ptr to document this ownership transfer?


>  }
>  static bool regionWithoutLoops(Region &R, LoopInfo *LI) {
>    for (const BasicBlock *BB : R.blocks())
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150604/38f8fcea/attachment.html>