[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
eduucaldas marked an inline comment as done. eduucaldas added inline comments. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:122 #endif + Node * = BeforeBegin ? BeforeBegin->NextSibling : FirstChild; + gribozavr2 wrote: > Could you move this definition up so that it can be used in the last assert > above? Done in a separate commit: 6fbad9bf304c05d37454420f7d5a1c2ab3adab20 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89303/new/ https://reviews.llvm.org/D89303 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4178f8f2f08e: [SyntaxTree] Improve safety of
`replaceChildRangeLowLevel` (authored by eduucaldas).
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,65 @@
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 * = 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;
+ 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 {
Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -94,48 +94,65 @@
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 * = 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
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:122 #endif + Node * = BeforeBegin ? BeforeBegin->NextSibling : FirstChild; + Could you move this definition up so that it can be used in the last assert above? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89303/new/ https://reviews.llvm.org/D89303 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
eduucaldas updated this revision to Diff 298079.
eduucaldas marked 2 inline comments as done.
eduucaldas added a comment.
Answer to comments
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,65 @@
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 * = 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;
+ 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 {
Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -94,48 +94,65 @@
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 * = 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
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
gribozavr2 added inline comments.
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:103
#ifndef NDEBUG
- for (auto *N = New; N; N = N->getNextSibling()) {
+ for (auto *N = New; N; N = N->NextSibling) {
assert(N->Parent == nullptr);
eduucaldas wrote:
> Throughout the function we use data members instead of accessors. Is one
> preferrable to the other?
I don't think there is a difference.
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:124
+return BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+ };
+
Why lambda and not:
```
Node * = BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89303/new/
https://reviews.llvm.org/D89303
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
eduucaldas added a reviewer: gribozavr2.
eduucaldas added inline comments.
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:103
#ifndef NDEBUG
- for (auto *N = New; N; N = N->getNextSibling()) {
+ for (auto *N = New; N; N = N->NextSibling) {
assert(N->Parent == nullptr);
Throughout the function we use data members instead of accessors. Is one
preferrable to the other?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89303/new/
https://reviews.llvm.org/D89303
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
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 = [, = 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 {
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 = [, = 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)
-
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
eduucaldas updated this revision to Diff 297800.
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,65 @@
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 = [, = this->FirstChild]() -> Node *& {
+return BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+ };
// 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;
}
- // Attach new nodes.
- if (BeforeBegin)
-BeforeBegin->NextSibling = New ? New : End;
- else
-FirstChild = New ? New : End;
+ // Mark modification.
+ if (New || GetBegin() != End)
+for (auto *T = this; T && T->Original; T = T->Parent)
+ T->Original = false;
- if (New) {
-auto *Last = New;
-for (auto *N = New; N != nullptr; N = N->getNextSibling()) {
- Last = N;
- N->Parent = this;
-}
-Last->NextSibling = End;
+ if (!New) {
+GetBegin() = End;
+return;
}
-
- // Mark the node as modified.
- for (auto *T = this; T && T->Original; T = T->Parent)
-T->Original = false;
+ // Attach new nodes.
+ GetBegin() = New;
+ auto *Last = New;
+ for (auto *N = New; N != nullptr; N = N->NextSibling) {
+Last = N;
+N->Parent = this;
+ }
+ Last->NextSibling = End;
}
namespace {
Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -94,48 +94,65 @@
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 = [, = this->FirstChild]() -> Node *& {
+return BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+ };
// 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;
}
- // Attach
[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`
eduucaldas created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
eduucaldas requested review of this revision.
- Add assertions for other preconditions.
- If nothing is modified, don't mark it.
Repository:
rG LLVM Github Monorepo
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,65 @@
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 = [, = this->FirstChild]() -> Node *& {
+return BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+ };
// 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;
}
- // Attach new nodes.
- if (BeforeBegin)
-BeforeBegin->NextSibling = New ? New : End;
- else
-FirstChild = New ? New : End;
+ // Mark modification.
+ if (New || GetBegin() != End)
+for (auto *T = this; T && T->Original; T = T->Parent)
+ T->Original = false;
- if (New) {
-auto *Last = New;
-for (auto *N = New; N != nullptr; N = N->getNextSibling()) {
- Last = N;
- N->Parent = this;
-}
-Last->NextSibling = End;
+ // Attach new nodes.
+ if (!New) {
+GetBegin() = End;
+return;
}
-
- // Mark the node as modified.
- for (auto *T = this; T && T->Original; T = T->Parent)
-T->Original = false;
+ GetBegin() = New;
+ auto *Last = New;
+ for (auto *N = New; N != nullptr; N = N->NextSibling) {
+Last = N;
+N->Parent = this;
+ }
+ Last->NextSibling = End;
}
namespace {
Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -94,48 +94,65 @@
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 = [, = this->FirstChild]() -> Node *& {
+return BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+ };
// 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; });
+
