[PATCH] D30803: [RegionInfo] Don't return an invalid pointer when removing a subregion

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg created this revision.
Herald added a subscriber: wdng.

RegionBase<Tr>::removeSubRegion removes a subregion from its children, then returns a pointer to the removed subregion.

Starting from r206310 <https://reviews.llvm.org/rL206310> (Use unique_ptr to manage ownership of child Regions within llvm::Region), the children are stored using a unique_ptr<RegionT>, which obviously deletes the region when the child is erased from the container, and returns an invalid pointer.

I see three solutions here:

1. Don't return the removed child.
2. Move the unique_ptr out of the container and return it. This would mean that we need to update all the other functions that take raw pointers and store them in unique_ptr internally (addSubRegion?).
3. Release the unique_ptr and "transfer" the ownership to the caller.

I chose the first one, since in my use-case I don't need the removed subregion anymore, since I'm using transferChildrenTo.


https://reviews.llvm.org/D30803

Files:
  include/llvm/Analysis/RegionInfo.h
  include/llvm/Analysis/RegionInfoImpl.h


Index: include/llvm/Analysis/RegionInfoImpl.h
===================================================================
--- include/llvm/Analysis/RegionInfoImpl.h
+++ include/llvm/Analysis/RegionInfoImpl.h
@@ -398,16 +398,15 @@
 }
 
 template <class Tr>
-typename Tr::RegionT *RegionBase<Tr>::removeSubRegion(RegionT *Child) {
+void RegionBase<Tr>::removeSubRegion(RegionT *Child) {
   assert(Child->parent == this && "Child is not a child of this region!");
   Child->parent = nullptr;
   typename RegionSet::iterator I =
       find_if(children, [&](const std::unique_ptr<RegionT> &R) {
         return R.get() == Child;
       });
   assert(I != children.end() && "Region does not exit. Unable to remove.");
-  children.erase(children.begin() + (I - begin()));
-  return Child;
+  children.erase(I);
 }
 
 template <class Tr>
Index: include/llvm/Analysis/RegionInfo.h
===================================================================
--- include/llvm/Analysis/RegionInfo.h
+++ include/llvm/Analysis/RegionInfo.h
@@ -516,10 +516,9 @@
 
   /// @brief Remove a subregion from this Region.
   ///
-  /// The subregion is not deleted, as it will probably be inserted into another
-  /// region.
-  /// @param SubRegion The SubRegion that will be removed.
-  RegionT *removeSubRegion(RegionT *SubRegion);
+  /// @param SubRegion The SubRegion that will be removed this region's children
+  /// and deleted.
+  void removeSubRegion(RegionT *SubRegion);
 
   /// @brief Move all direct child nodes of this Region to another Region.
   ///


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D30803.91250.patch
Type: text/x-patch
Size: 1530 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170310/f7a17f56/attachment.bin>