[SyntaxTree] Improve safety of `replaceChildRangeLowLevel`

[eduucaldas]

Summary

• Add assertions for other preconditions.
• If nothing is modified, don't mark it.

[eduucaldas]

minor

[eduucaldas]

minor

[eduucaldas]

clang/lib/Tooling/Syntax/Tree.cpp

103

Throughout the function we use data members instead of accessors. Is one preferrable to the other?

[gribozavr2]

clang/lib/Tooling/Syntax/Tree.cpp

103

I don't think there is a difference.

124

Why lambda and not:

Node *&Begin = BeforeBegin ? BeforeBegin->NextSibling : FirstChild;

[eduucaldas]

Answer to comments

[gribozavr2]

clang/lib/Tooling/Syntax/Tree.cpp

122

Could you move this definition up so that it can be used in the last assert above?

[eduucaldas]

clang/lib/Tooling/Syntax/Tree.cpp

122

Done in a separate commit: 6fbad9bf304c05d37454420f7d5a1c2ab3adab20

clang/lib/Tooling/Syntax/Tree.cpp

void syntax::Tree::prependChildLowLevel(Node *Child) {
 
   Child->Parent = this;
   Child->NextSibling = this->FirstChild;
   this->FirstChild = Child;
 }
 
 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
+  Node *&Begin = BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+
+  if (!New && Begin == 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 = Begin; 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) {
+    Begin = 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;
-  }
+  Begin = New;
+  for (auto *N = New; N != nullptr; N = N->NextSibling) {
 
-  // Mark the node as modified.
-  for (auto *T = this; T && T->Original; T = T->Parent)
-    T->Original = false;
-}
-
 namespace {
 static void dumpLeaf(raw_ostream &OS, const syntax::Leaf *L,
                      const SourceManager &SM) {
   assert(L);
   const auto *Token = L->getToken();
   assert(Token);
   // Handle 'eof' separately, calling text() on it produces an empty string.
   if (Token->kind() == tok::eof)