[PATCH] D55257: Inline handling of DependentSizedArrayType

[Aaron Ballman via Phabricator via cfe-commits]

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