[SyntaxTree] Implement "by-pointer output parameter to return value" refactoring.

[eduucaldas]

Summary

Example of such refactoring:

Customer* C;
getCustumer(C);

Customer C = getCustumer();

[dblaikie]

I imagine test coverage would be handy (I'm probably not the right reviewer for this in general, but was/am curious to see how it goes - you might want to search for other contributors/reviewers to the code you're changing and request/add them as reviewers for this patch)

[eduucaldas]

This patch will build upon https://reviews.llvm.org/D90240 and https://reviews.llvm.org/D90161
When those patches land, work on this patch will resume.
It is here to illustrate the relevance of the previous patches and the general direction we're going.

Glad to see that you're interested David, we'll keep you updated.

[eduucaldas]

Sam, this patch is outdated, as we're still making decisions on https://reviews.llvm.org/D90161 and we haven't yet landed https://reviews.llvm.org/D90240, please don't bother reviewing it.

This is where we started thinking about iterators on lists, and about the usefulness of having reverse links for syntax nodes.

I have added you to the subscriber list so you can follow the discussion once this is unblocked, and to show you the Mutations API in its early steps.

clang/include/clang/Tooling/Syntax/Mutations.h

 
 /// Removes a statement or replaces it with an empty statement where one is
 /// required syntactically. E.g., in the following example:
 ///     if (cond) { foo(); } else bar();
 /// One can remove `foo();` completely and to remove `bar();` we would need to
 /// replace it with an empty statement.
 /// EXPECTS: S->canModify() == true
 void removeStatement(syntax::Arena &A, syntax::Statement *S);
+void replaceOutputParameterWithReturnValue(syntax::Arena &A,
+                                           syntax::CallExpression *CE);
 
 } // namespace syntax
 } // namespace clang
 
 #endif

clang/lib/Tooling/Syntax/Mutations.cpp

static void remove(syntax::Node *N) {
 
     auto *P = N->getParent();
     P->replaceChildRangeLowLevel(findPrevious(N), N->getNextSibling(),
                                  /*New=*/nullptr);
 
     P->assertInvariants();
     N->assertInvariants();
   }
+
+  static void
+  replaceChildRange(syntax::Tree *Parent, syntax::Node *BeforeBegin,
+                    syntax::Node *End,
+                    ArrayRef<std::pair<Node *, NodeRole>> NewRange) {
+    if (NewRange.empty())
+      return Parent->replaceChildRangeLowLevel(BeforeBegin, End, nullptr);
+
+    for (const auto &NodeAndRole : NewRange)
+      NodeAndRole.first->setRole(NodeAndRole.second);
+
+    for (auto It = NewRange.begin(), EndIt = std::prev(NewRange.end());
+         It != EndIt; ++It) {
+      auto *Node = It->first;
+      auto *Next = std::next(It)->first;
+      Node->NextSibling = Next;
+    }
+
+    Parent->replaceChildRangeLowLevel(BeforeBegin, End, NewRange.front().first);
+  }
+
+  /// Removes all the `Node`s between the iterators `BeforeBegin` and `End`. For
+  /// separated lists, if we remove the last element we may need to remove the
+  /// delimiter before it, in order to keep it a separated list.
+  static void removeElementAndDelimiterRange(
+      List::ElementAndDelimiterIterator<Node> BeforeBegin,
+      List::ElementAndDelimiterIterator<Node> End) {
+    auto *EndNode = [&End]() -> Node * {
+      if (End.isEnd())
+        return nullptr;
+      if ((*End).element)
+        return (*End).element;
+      return (*End).delimiter;
+    }();
+    auto *Parent = End.getParent();
+    Node *BeforeBeginNode;
+    if (End.isEnd() &&
+        Parent->getTerminationKind() == List::TerminationKind::Separated) {
+      // In this case we also remove the delimiter before the last element of
+      // the list, if it exists.
+      if (BeforeBegin.isBeforeBegin())
+        BeforeBeginNode = nullptr;
+      else if ((*BeforeBegin).element)
+        BeforeBeginNode = (*BeforeBegin).element;
+      else if ((*BeforeBegin).delimiter)
+        BeforeBeginNode = findPrevious((*BeforeBegin).delimiter);
+      else
+        // `BeforeBegin` = (null, null) and thus we are in the last element of
+        // the list. Nothing will be removed.
+        return;
+    } else
+      BeforeBeginNode = [&BeforeBegin]() -> Node * {
+        if (BeforeBegin.isBeforeBegin())
+          return nullptr;
+        if ((*BeforeBegin).delimiter)
+          return (*BeforeBegin).delimiter;
+        if ((*BeforeBegin).element)
+          return (*BeforeBegin).element;
+        llvm_unreachable("(null, null) can only appear in the end of a "
+                         "separated range, and this is not possible here");
+      }();
+
+    Parent->replaceChildRangeLowLevel(BeforeBeginNode, EndNode, nullptr);
+  }
+
+  static Node *extractLastElementAndDelimiter(List *Parent) {
+    auto Prev = [](List::ElementAndDelimiterIterator<Node> EDI,
+                   unsigned i = 1) {
+      auto Previous = EDI.getParent()->getElementAndDelimiterBeforeBegin();
+      auto It = Previous;
+      for (unsigned j = 0; j < i; ++j)
+        ++It;
+      for (; It != EDI; ++It)
+        ++Previous;
+
+      return Previous;
+    };
+    if (Parent->getElementAndDelimiterBegin() ==
+        Parent->getElementAndDelimiterEnd())
+      return nullptr;
+    auto BeforeLast = Prev(Parent->getElementAndDelimiterEnd(), 2);
+    auto Last = std::next(BeforeLast);
+    removeElementAndDelimiterRange(BeforeLast,
+                                   Parent->getElementAndDelimiterEnd());
+    return (*Last).element;
+  }
 };
 
 void syntax::removeStatement(syntax::Arena &A, syntax::Statement *S) {
   assert(S);
   assert(S->canModify());
 
   if (isa<CompoundStatement>(S->getParent())) {
     // A child of CompoundStatement can just be safely removed.
     MutationsImpl::remove(S);
     return;
   }
   // For the rest, we have to replace with an empty statement.
   if (isa<EmptyStatement>(S))
     return; // already an empty statement, nothing to do.
 
   MutationsImpl::replace(S, createEmptyStatement(A));
 }
+
+void syntax::replaceOutputParameterWithReturnValue(syntax::Arena &A,
+                                                   syntax::CallExpression *CE) {
+  assert(CE);
+  assert(CE->canModify());
+  assert(CE->getParent());
+
+  // Extract Output Parameter
+  auto *OutParam =
+      MutationsImpl::extractLastElementAndDelimiter(CE->getArguments());
+
+  // Create pointer dereference with outParam
+  auto *DerefOutParam =
+      createTree(A,
+                 {{createLeaf(A, tok::star), NodeRole::OperatorToken},
+                  {OutParam, NodeRole::Operand}},
+                 NodeKind::PrefixUnaryOperatorExpression);
+
+  // f(); -> *outParam = f();
+  // TODO: How to improve this: Add parent to AddAfter. Or `NodeIterator`, with
+  // a pointer to Parent when it's a sentinel.
+  auto *Parent = CE->getParent();
+  auto *BeforeBegin = findPrevious(CE);
+  auto *End = CE->getNextSibling();
+  auto Role = CE->getRole();
+  MutationsImpl::remove(CE);
+  auto *Assignment =
+      createTree(A,
+                 {{DerefOutParam, NodeRole::LeftHandSide},
+                  {createLeaf(A, tok::equal), NodeRole::OperatorToken},
+                  {CE, NodeRole::RightHandSide}},
+                 NodeKind::BinaryOperatorExpression);
+
+  MutationsImpl::replaceChildRange(Parent, BeforeBegin, End,
+                                   {{Assignment, Role}});
+}