[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; }); +