[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-27 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:352 + default: +llvm_unreachable("List children can only be elements or delimiters."); } eduucaldas wrote: > sammccall wrote: > > Let's avoid deliberately crashing on invalid c

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-27 Thread Eduardo Caldas via Phabricator via cfe-commits
eduucaldas updated this revision to Diff 301024. eduucaldas marked 6 inline comments as done. eduucaldas added a comment. - `const` on `getElement` and similar. - `NotSentinel` -> `Element` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90161/new/ h

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-27 Thread Eduardo Caldas via Phabricator via cfe-commits
eduucaldas marked 8 inline comments as done. eduucaldas added a comment. I left some points unanswered, I'll answer them tomorrow :) Comment at: clang/include/clang/Tooling/Syntax/Tree.h:231 + /// `ElementAndDelimiter` acts as one. + template class ElementAndDelimiterIterato

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment. This seems like more implementation and API complexity than is justified by the problems it's solving. I do think it's possible to drastically simplify it (I've left some suggestions here) but if not, I think we should move it to its own header. Com

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits
eduucaldas updated this revision to Diff 300702. eduucaldas added a comment. Diff against master, sorry about that Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90161/new/ https://reviews.llvm.org/D90161 Files: clang/include/clang/Tooling/Syntax

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment. (I started to review this, but now the current diff has only your changes against the first diff, rather than against master - can you upload a new diff?) Comment at: clang/include/clang/Tooling/Syntax/Tree.h:231 + /// `ElementAndDelimiter` acts as

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits
eduucaldas updated this revision to Diff 300696. eduucaldas added a comment. Fix comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90161/new/ https://reviews.llvm.org/D90161 Files: clang/include/clang/Tooling/Syntax/Tree.h clang/unittests

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits
eduucaldas marked an inline comment as done. eduucaldas added a comment. In D90161#2353736 , @gribozavr2 wrote: > Could you add tests that verify the pairing of elements and delimiters? Those are surfaced through the previous tests, via `getElementsAsNod

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Could you add tests that verify the pairing of elements and delimiters? Comment at: clang/include/clang/Tooling/Syntax/Tree.h:214 + /// elements and delimiters are represented as null pointers. Below we give + /// examples of what this iteration wo

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits
eduucaldas created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. eduucaldas requested review of this revision. Provide an iterator for `List` that iterates through elements and delimiters on pairs. This iterator is robust to missing elements. For instance,