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

[thegameg]

Summary

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

Starting from r206310 (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.

[MatzeB]

Looks like nobody uses the return value anyway. LGTM.

[thegameg]

Fix typo in comment.

include/llvm/Analysis/RegionInfo.h

#endif
   ///
   /// @param SubRegion The new subregion that will be added.
   /// @param moveChildren Move the children of this region, that are also
   ///                     contained in SubRegion into SubRegion.
   void addSubRegion(RegionT *SubRegion, bool moveChildren = false);
 
   /// @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 from this region's
+  /// children, and then deleted.
+  void removeSubRegion(RegionT *SubRegion);
 
   /// @brief Move all direct child nodes of this Region to another Region.
   ///
   /// @param To The Region the child nodes will be transferred to.
   void transferChildrenTo(RegionT *To);
 
   /// @brief Verify if the region is a correct region.
   ///

include/llvm/Analysis/RegionInfoImpl.h

void RegionBase<Tr>::addSubRegion(RegionT *SubRegion, bool moveChildren) {
   children.clear();
   children.insert(
       children.begin(),
       std::move_iterator<typename RegionSet::iterator>(Keep.begin()),
       std::move_iterator<typename RegionSet::iterator>(Keep.end()));
 }
 
 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>
 unsigned RegionBase<Tr>::getDepth() const {
   unsigned Depth = 0;
 
   for (RegionT *R = getParent(); R != nullptr; R = R->getParent())
     ++Depth;