steveire added a comment. 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. 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
