gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:182
+ /// The element, or nullptr if past-the-end.
+ NodeT *asPointer() const { return N; }
+ };
----------------
"nodePointer()" ?
================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:202-219
+ /// child_iterator is not invalidated by mutations.
+ struct child_iterator : child_iterator_base<child_iterator, Node> {
+ using Base::Base;
+ };
+ struct const_child_iterator
+ : child_iterator_base<const_child_iterator, const Node> {
+ using Base::Base;
----------------
sammccall wrote:
> eduucaldas wrote:
> > TL;DR:
> > `child_iterator` -> `ChildIterator`
> > `const_child_iterator` -> `ConstChildIterator`
> > `children` -> `getChildren`
> >
> > I see you followed the naming convention of the ClangAST, which makes loads
> > of sense.
> >
> > In syntax trees we follow the naming conventions of the [conding
> > standard](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
> > -- just because we want consistency and we had to pick a guideline to
> > follow -- according to that, functions should be verb like and names should
> > be camel case.
> >
> > If like us you chose `const_child_iterator` and `children` kind of "just
> > because", could you change it to follow `ConstChildIterator` and
> > `getChildren` respectively.
> > In syntax trees we follow the naming conventions of the conding standard
>
> I see that's been used in some of the newer classes, and that you changed
> this file to follow that style in 4c14ee61b73746b314d83e7c52e03d6527b78105.
> However it's not the original style we used here and elsewhere in libSyntax.
> It's a bit frustrating to hear the argument about consistency, because we
> were quite careful and deliberate about this style, which was IMO fairly
> consistent prior to that change.
>
> I'm willing to change these to match the local style in this file. However
> `*_iterator`, `*_begin/end/range()` is more common than FooIterator etc in
> LLVM overall (I think for consistency with `iterator` etc, which is endorsed
> by the style guide). So I don't think this change is particularly an
> improvement, even in terms of consistency.
It can go either way:
> As an exception, classes that mimic STL classes can have member names in
> STL’s style of lower-case words separated by underscores
================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:210
+ const_child_iterator(const child_iterator &I)
+ : Base(I == child_iterator() ? nullptr : &*I) {}
+ };
----------------
================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:22-23
if (auto *T = dyn_cast<syntax::Tree>(N)) {
- for (const auto *C = T->getFirstChild(); C; C = C->getNextSibling())
- traverse(C, Visit);
+ for (const syntax::Node &C : T->children())
+ traverse(&C, Visit);
}
----------------
sammccall wrote:
> eduucaldas wrote:
> > Hmm...
> > That looks funny, if the user uses a range-based for loop and forgets the
> > `&`, then we would be copying the Node???
> >
> > Also in many places we had to get the address to the node. That is not
> > really consistent with how the syntax trees API's were designed, they
> > generally take pointers instead of references. (that said we could
> > obviously reconsider those design choices)
> > That looks funny, if the user uses a range-based for loop and forgets the
> > &, then we would be copying the Node???
>
> It doesn't look funny to me - foreach usually iterates over values rather
> than pointers. This raises a good point - it looks like Node is copyable and
> shouldn't be. (The default copy operation violates the tree invariants, e.g.
> the copied node will not be a child of its parent). I'll send a patch...
>
> (That said this is always the case with copyable objects in range-based for
> loops, and indeed using references - that doesn't usually mean we avoid using
> them)
>
> > Also in many places we had to get the address to the node. That is not
> > really consistent with how the syntax trees API's were designed, they
> > generally take pointers instead of references
>
> I'm not convinced that taking the address is itself a burden, any more than
> using `->` instead of `.` is a burden.
> Sometimes the use of pointer vs reference intends to convey something about
> address stability, but here this goes with the type as all nodes are
> allocated on the arena. (IMO this is a part of the AST design that works
> well).
> The other thing it conveys is nullability, and this seems valuable enough
> that IMO APIs that take *non-nullable* nodes by pointer should be
> reconsidered.
>
> For the iterators specifically, the main alternative here I guess is having
> Tree::children() act as a container of *pointers to* children, instead of the
> children themselves. This *is* highly unconventional and surprising. Consider
> the ubiquitous `for (const auto&N : T->children())`... now the author/reader
> expects N to be a const reference to a child, instead it's a const reference
> to a **non-const** pointer to a child....
> That looks funny, if the user uses a range-based for loop and forgets the &,
> then we would be copying the Node???
Aren't they noncopyable?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90023/new/
https://reviews.llvm.org/D90023
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
