NFC: Simplify dumpStmt child handling

[aaron.ballman]

lib/AST/ASTDumper.cpp

1987 ↗

(On Diff #175911)

Was there something special about calling the Visit methods directly and bailing out? Your code certainly looks reasonable, but I wonder if the output is changed because it goes through the ConstStmtVisitor instead of directly dispatching.

1990 ↗

(On Diff #175911)

These should be isa<> checks instead of dyn_cast<> checks.

[steveire]

Use isa instead of dyn_cast

[steveire]

lib/AST/ASTDumper.cpp

1987 ↗

(On Diff #175911)

I don't think it could have changed. By my understanding of the StmtVisitor, this has the same effect.

[aaron.ballman]

lib/AST/ASTDumper.cpp

1987 ↗

(On Diff #175911)

Yeah, I don't see how it would either, but the original code smells suspicious to me. How about adding some tests for DeclStmt and GenericSelectionExpr that demonstrates explicitly this isn't changing behavior?

In fact, it seems that we have some bugs in this area (with DeclStmt) already, so understanding what's changing may point out fixes. https://godbolt.org/z/tDJ-0H

[steveire]

lib/AST/ASTDumper.cpp

1987 ↗

(On Diff #175911)

A visual inspection and reading of the visitor should show that this patch is NFC. I'd welcome a test if you can make one. The test should be in the repo before this commit anyway.

In fact, it seems that we have some bugs in this area (with DeclStmt) already, so understanding what's changing may point out fixes. https://godbolt.org/z/tDJ-0H

That is not related to this refactoring.

Also, clang-query has different output:

http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/W_kZFl

[steveire]

Aaron, you added tests for the existing behavior which pass after this patch. Is there anything holding it up?

[aaron.ballman]

In D55068#1315924, @steveire wrote:

Aaron, you added tests for the existing behavior which pass after this patch. Is there anything holding it up?

Nope, I was just waiting on confirmation that the behavior hadn't changed. Thanks for letting me know!

LGTM!

cfe/trunk/lib/AST/ASTDumper.cpp

 void ASTDumper::dumpStmt(const Stmt *S) {
   dumpChild([=] {
     if (!S) {
       ColorScope Color(OS, ShowColors, NullColor);
       OS << "<<<NULL>>>";
       return;
     }
 
+    ConstStmtVisitor<ASTDumper>::Visit(S);
+
     // Some statements have custom mechanisms for dumping their children.
-    if (const DeclStmt *DS = dyn_cast<DeclStmt>(S)) {
-      VisitDeclStmt(DS);
-      return;
-    }
-    if (const GenericSelectionExpr *GSE = dyn_cast<GenericSelectionExpr>(S)) {
-      VisitGenericSelectionExpr(GSE);
+    if (isa<DeclStmt>(S) || isa<GenericSelectionExpr>(S)) {
       return;
     }
 
-    ConstStmtVisitor<ASTDumper>::Visit(S);
-
     for (const Stmt *SubStmt : S->children())
       dumpStmt(SubStmt);
   });
 }
 
 void ASTDumper::VisitStmt(const Stmt *Node) {
   {
     ColorScope Color(OS, ShowColors, StmtColor);