[PATCH] D55257: Inline handling of DependentSizedArrayType

[Stephen Kelly via Phabricator via cfe-commits]

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