Inline handling of DependentSizedArrayType

[steveire]

Summary

Re-order handling of getElementType and getBracketsRange. It is
necessary to perform all printing before any traversal to child nodes.

This causes no change in the output of ast-dump-array.cpp due to the way
child nodes are printed with a delay. This new order of the code is
also the order that produces the expected output anyway.

[aaron.ballman]

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"?

[steveire]

In 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 ?

[aaron.ballman]

In D55257#1318376, @steveire wrote:

In 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?)

[steveire]

In D55257#1318769, @aaron.ballman wrote:

In D55257#1318376, @steveire wrote:

In 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.

[aaron.ballman]

In D55257#1319016, @steveire wrote:

In D55257#1318769, @aaron.ballman wrote:

In D55257#1318376, @steveire wrote:

In 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!

lib/AST/ASTDumper.cpp

public:
     }
     void VisitVariableArrayType(const VariableArrayType *T) {
       OS << " ";
       dumpSourceRange(T->getBracketsRange());
       VisitArrayType(T);
       dumpStmt(T->getSizeExpr());
     }
     void VisitDependentSizedArrayType(const DependentSizedArrayType *T) {
-      VisitArrayType(T);
+      switch (T->getSizeModifier()) {
+        case ArrayType::Normal: break;
+        case ArrayType::Static: OS << " static"; break;
+        case ArrayType::Star: OS << " *"; break;
+      }
+      OS << " " << T->getIndexTypeQualifiers().getAsString();
       OS << " ";
       dumpSourceRange(T->getBracketsRange());
+      dumpTypeAsChild(T->getElementType());
       dumpStmt(T->getSizeExpr());
     }
     void VisitDependentSizedExtVectorType(
         const DependentSizedExtVectorType *T) {
       OS << " ";
       dumpLocation(T->getAttributeLoc());
       dumpTypeAsChild(T->getElementType());
       dumpStmt(T->getSizeExpr());