Re-arrange content in FunctionDecl dump

[steveire]

Summary

Output all content which is local to the FunctionDecl before traversing
to child AST nodes.

This is necessary so that all of the part which is local to the FunctionDecl can be split into a different method.

[aaron.ballman]

The summary explains what it's doing, but not why it's doing it -- why is this change in behavior needed? Does this not break any tests?

Btw, as we work on this refactoring, I think a good approach will be to build up the base of tests around AST dumping so that we can be explicitly aware of any behavioral changes from the patches. We have some coverage, but it doesn't look to be particularly comprehensive. I am happy to contribute some of these tests. WDYT?

[steveire]

Yes, please commit your new tests for FunctionDecl dumping before this patch can go in.

[aaron.ballman]

In D55083#1314546, @steveire wrote:

Yes, please commit your new tests for FunctionDecl dumping before this patch can go in.

r347994 has those tests.

[steveire]

Adjust tests

[aaron.ballman]

test/AST/ast-dump-decl.cpp

217 ↗

(On Diff #177069)

I find this ordering to be confusing, especially because the template argument information is used by the parameter declaration type.

test/AST/ast-dump-funcs.cpp

59

I don't think this belongs at the start of the function -- the parameter information seems far more interesting than what the method overrides.

[steveire]

Update

[steveire]

test/AST/ast-dump-funcs.cpp

59

Output does not occur in order of 'interesting-ness'.

[aaron.ballman]

LGTM aside from some nit cleanup.

lib/AST/ASTDumper.cpp

648

Can switch to const auto *

649–651

Might as well switch this to use a range-based for loop over inits()

test/AST/ast-dump-funcs.cpp

59

Except that it kind of does occur in order of interesting-ness -- output occurs in whatever order is going to make the AST most understandable to the user.

My worry here is whether the parameter list will get "hidden" by the list of overrides, but I'm not certain this will be a problem in practice. I suspect there aren't a ton of deep class hierarchies with a lot of overrides for the same method across the entire hierarchy. I did some searching to see if I could find code examples in the wild where this output would be demonstrably worse, but I didn't find any, so I think this may be okay.

lib/AST/ASTDumper.cpp

case EST_Unevaluated:
       OS << " noexcept-unevaluated " << EPI.ExceptionSpec.SourceDecl;
       break;
     case EST_Uninstantiated:
       OS << " noexcept-uninstantiated " << EPI.ExceptionSpec.SourceTemplate;
       break;
     }
   }
 
-  if (const FunctionTemplateSpecializationInfo *FTSI =
-          D->getTemplateSpecializationInfo())
-    dumpTemplateArgumentList(*FTSI->TemplateArguments);
-
-  if (!D->param_begin() && D->getNumParams())
-    dumpChild([=] { OS << "<<NULL params x " << D->getNumParams() << ">>"; });
-  else
-    for (const ParmVarDecl *Parameter : D->parameters())
-      dumpDecl(Parameter);
-
-  if (const CXXConstructorDecl *C = dyn_cast<CXXConstructorDecl>(D))
-    for (const auto *I : C->inits())
-      dumpCXXCtorInitializer(I);
-
   if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D)) {
     if (MD->size_overridden_methods() != 0) {
       auto dumpOverride = [=](const CXXMethodDecl *D) {
         SplitQualType T_split = D->getType().split();
         OS << D << " " << D->getParent()->getName()
            << "::" << D->getNameAsString() << " '"
            << QualType::getAsString(T_split, PrintPolicy) << "'";
       };
 
       dumpChild([=] {
         auto Overrides = MD->overridden_methods();
         OS << "Overrides: [ ";
         dumpOverride(*Overrides.begin());
         for (const auto *Override :
              llvm::make_range(Overrides.begin() + 1, Overrides.end())) {
           OS << ", ";
           dumpOverride(Override);
         }
         OS << " ]";
       });
     }
   }
 
+  if (const FunctionTemplateSpecializationInfo *FTSI =
+          D->getTemplateSpecializationInfo())
+    dumpTemplateArgumentList(*FTSI->TemplateArguments);
+
+  if (!D->param_begin() && D->getNumParams())
+    dumpChild([=] { OS << "<<NULL params x " << D->getNumParams() << ">>"; });
+  else
+    for (const ParmVarDecl *Parameter : D->parameters())
+      dumpDecl(Parameter);
+
+  if (const CXXConstructorDecl *C = dyn_cast<CXXConstructorDecl>(D))
+    for (CXXConstructorDecl::init_const_iterator I = C->init_begin(),
+                                                 E = C->init_end();
+         I != E; ++I)
+      dumpCXXCtorInitializer(*I);
+
   if (D->doesThisDeclarationHaveABody())
     dumpStmt(D->getBody());
 }
 
 void ASTDumper::VisitFieldDecl(const FieldDecl *D) {
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   if (D->isMutable())

test/AST/ast-dump-funcs.cpp

struct S {
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'S &(const S &)' inline default_delete noexcept-unevaluated
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'S &(S &&)' inline default_delete noexcept-unevaluated
   // CHECK: CXXDestructorDecl 0x{{[^ ]*}} <col:8> col:8 implicit ~S 'void ()' inline default noexcept-unevaluated
 };
 
 struct T : S { // T is not referenced, but S is
   void f(float, int = 100) override;
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:28> col:8 f 'void (float, int)'
+  // CHECK-NEXT: Overrides: [ 0x{{[^ ]*}} S::f 'void (float, int)' ]
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}} <col:10> col:15 'float'
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}} <col:17, col:23> col:21 'int' cinit
   // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}} <col:23> 'int' 100
-  // CHECK-NEXT: Overrides: [ 0x{{[^ ]*}} S::f 'void (float, int)' ]
   // CHECK-NEXT: OverrideAttr
 
   // CHECK: CXXConstructorDecl 0x{{[^ ]*}} <line:[[@LINE-9]]:8> col:8 implicit T 'void (const T &)' inline default_delete noexcept-unevaluated
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'T &(const T &)' inline default_delete noexcept-unevaluated
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'T &(T &&)' inline default_delete noexcept-unevaluated
   // CHECK: CXXDestructorDecl 0x{{[^ ]*}} <col:8> col:8 implicit ~T 'void ()' inline default noexcept-unevaluated
 };