aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D55257#1319016 <https://reviews.llvm.org/D55257#1319016>, @steveire wrote: > In D55257#1318769 <https://reviews.llvm.org/D55257#1318769>, @aaron.ballman > wrote: > > > In D55257#1318376 <https://reviews.llvm.org/D55257#1318376>, @steveire > > wrote: > > > > > In D55257#1318328 <https://reviews.llvm.org/D55257#1318328>, > > > @aaron.ballman wrote: > > > > > > > > It is necessary to perform all printing before any traversal to child > > > > > nodes. > > > > > > > > This piqued my interest -- is `VisitFunctionDecl()` then incorrect > > > > because it streams output, then dumps parameter children, then dumps > > > > more output, then dumps override children? Or do you mean "don't > > > > interleave `VisitFoo()` calls with streaming output"? > > > > > > > > > Can you relate your question to https://reviews.llvm.org/D55083 ? > > > > > > Ah, I was looking at code before having fetched those changes, so perhaps > > my example is poor. Mostly, I'm wondering what you meant by "traversal to > > child nodes" -- do you mean: > > > > 1. it's bad to output to the stream, then dumpChild(), then output to the > > stream again > > 2. it's bad to output to the stream, then VisitFoo(), then output to the > > stream again > > 3. both #1 and #2 > > 4. neither #1 nor #2 > > > > (as in: when I'm doing a code review a few months from now, what should I > > be watching out for in this scenario?) > > > I don't think 'bad' is the right phrasing, but mostly your #1 is correct. > VisitFoo() methods often call dumpChild() and even if one doesn't today, it > can. > > The reason to re-order is that it enables the separation of traversal from > outputting. See the split for comments in https://reviews.llvm.org/D55190 and > see that `dumpComment` there calls `NodeDumper.Visit`. > > When the refactoring is finished, the traversal class will not have a output > stream member, so it won't be able to stream to output, so there is nothing > you need to keep in mind in 6 months. Fantastic, thank you for the clarification; LGTM! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55257/new/ https://reviews.llvm.org/D55257 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
