[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas updated this revision to Diff 297803.
eduucaldas added a comment.

minor


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89303/new/

https://reviews.llvm.org/D89303

Files:
  clang/lib/Tooling/Syntax/Tree.cpp


Index: clang/lib/Tooling/Syntax/Tree.cpp
===================================================================
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -94,48 +94,67 @@
 
 void syntax::Tree::replaceChildRangeLowLevel(Node *BeforeBegin, Node *End,
                                              Node *New) {
-  assert(!BeforeBegin || BeforeBegin->Parent == this);
+  assert((!BeforeBegin || BeforeBegin->Parent == this) &&
+         "`BeforeBegin` is not a child of `this`.");
+  assert((!End || End->Parent == this) && "`End` is not a child of `this`.");
+  assert(canModify() && "Cannot modify `this`.");
 
 #ifndef NDEBUG
-  for (auto *N = New; N; N = N->getNextSibling()) {
+  for (auto *N = New; N; N = N->NextSibling) {
     assert(N->Parent == nullptr);
     assert(N->getRole() != NodeRole::Detached && "Roles must be set");
     // FIXME: sanity-check the role.
   }
+
+  auto Reachable = [](Node *From, Node *N) {
+    if (!N)
+      return true;
+    for (auto *It = From; It; It = It->NextSibling)
+      if (It == N)
+        return true;
+    return false;
+  };
+  assert(Reachable(FirstChild, BeforeBegin) &&
+         "`BeforeBegin` is not reachable.");
+  assert(Reachable(BeforeBegin ? BeforeBegin->NextSibling : FirstChild, End) &&
+         "`End` is not after `BeforeBegin`.");
 #endif
+  auto GetBegin = [&BeforeBegin, &FirstChild = this->FirstChild]() -> Node *& {
+    return BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+  };
+
+  if (!New && GetBegin() == End)
+    return;
+
+  // Mark modification.
+  for (auto *T = this; T && T->Original; T = T->Parent)
+    T->Original = false;
 
   // Detach old nodes.
-  for (auto *N = !BeforeBegin ? FirstChild : BeforeBegin->getNextSibling();
-       N != End;) {
+  for (auto *N = GetBegin(); N != End;) {
     auto *Next = N->NextSibling;
 
     N->setRole(NodeRole::Detached);
     N->Parent = nullptr;
     N->NextSibling = nullptr;
     if (N->Original)
-      traverse(N, [&](Node *C) { C->Original = false; });
+      traverse(N, [](Node *C) { C->Original = false; });
 
     N = Next;
   }
 
+  if (!New) {
+    GetBegin() = End;
+    return;
+  }
   // Attach new nodes.
-  if (BeforeBegin)
-    BeforeBegin->NextSibling = New ? New : End;
-  else
-    FirstChild = New ? New : End;
-
-  if (New) {
-    auto *Last = New;
-    for (auto *N = New; N != nullptr; N = N->getNextSibling()) {
-      Last = N;
-      N->Parent = this;
-    }
-    Last->NextSibling = End;
+  GetBegin() = New;
+  auto *Last = New;
+  for (auto *N = New; N != nullptr; N = N->NextSibling) {
+    Last = N;
+    N->Parent = this;
   }
-
-  // Mark the node as modified.
-  for (auto *T = this; T && T->Original; T = T->Parent)
-    T->Original = false;
+  Last->NextSibling = End;
 }
 
 namespace {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89303.297803.patch
Type: text/x-patch
Size: 2797 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201013/a8016fb4/attachment-0001.bin>