Re-order type param children of ObjC nodes

[aaron.ballman]

I don't know enough about ObjC to feel comfortable saying whether this is a reasonable reordering or not, so adding some reviewers.

test/AST/ast-dump-decl.m

90 ↗

(On Diff #177071)

It seems strange to me to print out the type parameter after the superclass information given the source order. My understanding of the AST dumping order is that we try to keep the order of nodes in source order whenever possible.

[steveire]

test/AST/ast-dump-decl.m

90 ↗

(On Diff #177071)

That is not really a possible thing to try to do, because the AST dump doesn't relate to a single language. It should be seen as language independent.

The principle I'm follow is that nodes dump themselves in entirety before starting to dump their child nodes. That is a principle already followed by most nodes. Changing this seems to be low cost, low impact and high benefit to the code.

[dblaikie]

test/AST/ast-dump-decl.m

90 ↗

(On Diff #177071)

That is not really a possible thing to try to do, because the AST dump doesn't relate to a single language. It should be seen as language independent.

Is this particular aspect different between the different source languages Clang supports? (could you give examples?)

[steveire]

test/AST/ast-dump-decl.m

90 ↗

(On Diff #177071)

Hmm, maybe that wasn't a good point, particularly because these methods relate to ObjC.

Other languages (eg Go https://gobyexample.com/functions) have different order of param types and param names, and different order of params and return types etc. So, the more general AST nodes have less reason for their order based on 'the order in the source'.

Anyway, the principle I'm following is 'the node dumps itself before dumping its children'. That's the principle that will allow separating traversal from output.

[aaron.ballman]

Adding @rjmccall as an Obj-C expert to see if he has opinions on the output changes. Also, pinging @rsmith in case he'd like to weigh in.

I think the change in behavior here is reasonable, especially because it eases the transition to a more generic AST dump traverser. If Richard and John don't have strong objections (or don't come back with opinions) in the next week or two, I think we should move this forward.

test/AST/ast-dump-decl.m

90 ↗

(On Diff #177071)

I'm not opposed to changing the order of the nodes so long as the resulting output is still sufficiently clear. This test case looks reasonable to me, but it also has very little information printed between the ObjCInterfaceDecl node and the ObjCTypeParamDecl node. I don't know whether it's common to have long protocol lists in ObjC or not where that distance will widen to the point of being hard to understand the relationship between the two nodes.

[rjmccall]

I don't really have an opinion about this, sorry.

[aaron.ballman]

In D55394#1356837, @rjmccall wrote:

I don't really have an opinion about this, sorry.

Do you think ObjC users will find the new output to be harder to read or less understandable than the old output?

[rjmccall]

This is the AST dumper. This is not a user feature.

[rjmccall]

If I'm debugging a clang bug and calling D->dump(), I think it'll be just as clear as it used to be what the ObjCTypeParamDecl lines mean.

[aaron.ballman]

In D55394#1356941, @rjmccall wrote:

This is the AST dumper. This is not a user feature.

Clang developers are users, too. However, the AST dumping does get used a bit by non-Clang developers too (such as people using clang-query, who see AST output).

If I'm debugging a clang bug and calling D->dump(), I think it'll be just as clear as it used to be what the ObjCTypeParamDecl lines mean.

Fantastic, that's the key bit. Thank you for weighing in!

If John doesn't think the change in output will cause confusion for people more familiar with ObjC, then I'm good with these changes.

cfe/trunk/lib/AST/ASTDumper.cpp

void ASTDumper::VisitObjCTypeParamDecl(const ObjCTypeParamDecl *D) {
   if (D->hasExplicitBound())
     OS << " bounded";
   NodeDumper.dumpType(D->getUnderlyingType());
 }
 
 void ASTDumper::VisitObjCCategoryDecl(const ObjCCategoryDecl *D) {
   NodeDumper.dumpName(D);
   NodeDumper.dumpDeclRef(D->getClassInterface());
-  dumpObjCTypeParamList(D->getTypeParamList());
   NodeDumper.dumpDeclRef(D->getImplementation());
   for (ObjCCategoryDecl::protocol_iterator I = D->protocol_begin(),
                                            E = D->protocol_end();
        I != E; ++I)
     NodeDumper.dumpDeclRef(*I);
+  dumpObjCTypeParamList(D->getTypeParamList());
 }
 
 void ASTDumper::VisitObjCCategoryImplDecl(const ObjCCategoryImplDecl *D) {
   NodeDumper.dumpName(D);
   NodeDumper.dumpDeclRef(D->getClassInterface());
   NodeDumper.dumpDeclRef(D->getCategoryDecl());
 }
 
 void ASTDumper::VisitObjCProtocolDecl(const ObjCProtocolDecl *D) {
   NodeDumper.dumpName(D);
 
   for (auto *Child : D->protocols())
     NodeDumper.dumpDeclRef(Child);
 }
 
 void ASTDumper::VisitObjCInterfaceDecl(const ObjCInterfaceDecl *D) {
   NodeDumper.dumpName(D);
-  dumpObjCTypeParamList(D->getTypeParamListAsWritten());
   NodeDumper.dumpDeclRef(D->getSuperClass(), "super");
 
   NodeDumper.dumpDeclRef(D->getImplementation());
   for (auto *Child : D->protocols())
     NodeDumper.dumpDeclRef(Child);
+  dumpObjCTypeParamList(D->getTypeParamListAsWritten());
 }
 
 void ASTDumper::VisitObjCImplementationDecl(const ObjCImplementationDecl *D) {
   NodeDumper.dumpName(D);
   NodeDumper.dumpDeclRef(D->getSuperClass(), "super");
   NodeDumper.dumpDeclRef(D->getClassInterface());
   for (ObjCImplementationDecl::init_const_iterator I = D->init_begin(),
                                                    E = D->init_end();

cfe/trunk/test/AST/ast-dump-decl.m

 // CHECK-NEXT:   ObjCCategoryImpl{{.*}} 'TestObjCCategoryDecl'
 // CHECK-NEXT:   ObjCProtocol{{.*}} 'P'
 // CHECK-NEXT:   ObjCMethodDecl{{.*}} bar
 
 @interface TestGenericInterface<T> : A<P> {
 }
 @end
 // CHECK:      ObjCInterfaceDecl{{.*}} TestGenericInterface
-// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}} <col:33> col:33 T 'id':'id'
 // CHECK-NEXT:   -super ObjCInterface {{.+}} 'A'
 // CHECK-NEXT:   -ObjCProtocol {{.+}} 'P'
+// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}} <col:33> col:33 T 'id':'id'
 
 @implementation TestObjCClass (TestObjCCategoryDecl)
 - (void) bar {
 }
 @end
 // CHECK:      ObjCCategoryImplDecl{{.*}} TestObjCCategoryDecl
 // CHECK-NEXT:   ObjCInterface{{.*}} 'TestObjCClass'
 // CHECK-NEXT:   ObjCCategory{{.*}} 'TestObjCCategoryDecl'