Move decl context dumping to TextNodeDumper

[steveire]

Summary

Only an obscure case is moved.

[aaron.ballman]

It appears we have no test coverage for this case -- can you introduce some?

lib/AST/ASTDumper.cpp

507–508

Might as well clean this up to be if (const auto *DC = dyn_cast<DeclContext>(D))

514–515

Why did this condition get dropped?

1233

Why is the isa<> test needed? D must be an ObjCMethodDecl which inherits from DeclContext, so this seems unneeded.

lib/AST/TextNodeDumper.cpp

260

const auto *

262

const auto *

[steveire]

I don't know what C++ code results in the undeserialized declarations output. Can you suggest some?

lib/AST/ASTDumper.cpp

514–515

The condition is using internal knowledge of the dumpDeclContext method - that it doesn't do anything if these conditions are not met, so avoid calling the method in the first place. We can just call the method. The for loop which remains will simply do nothing.

[aaron.ballman]

In D56829#1361465, @steveire wrote:

I don't know what C++ code results in the undeserialized declarations output. Can you suggest some?

I haven't the foggiest idea. I'd recommend doing an svn blame to see what commit added that code and try to work backwards from there -- presumably there's some related test code that could be run under -ast-dump.

lib/AST/ASTDumper.cpp

514–515

Ah, I see what's happening now, thank you.

[steveire]

Update

[steveire]

Update

[aaron.ballman]

LGTM aside from the testing situation -- so long as the tests come out without changes, this is good to go. If there is a change in test behavior, let's do another round of review just to verify everyone's happy with the behavioral change.

lib/AST/ASTDumper.cpp

1233

Since this is just formatting changes unrelated to the patch, feel free to revert and commit separately for commit hygiene.

[steveire]

Update

[aaron.ballman]

LGTM! At some point, we may want to revisit how we dump TranslationUnitDecl to make it a bit less inscrutable (I have no idea what those "invalid sloc" strings are telling me). At that point, we may want to turn <undeserialized declarations> into undeserialized_declarations as well so it stands out a bit more.

[stella.stamenova]

It looks like this change broke a number of the LLDB tests:

lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/781

Could you look into this or revert the change, so that the bot can go back to green?

lib/AST/ASTDumper.cpp

 }
 
 void ASTDumper::dumpDeclContext(const DeclContext *DC) {
   if (!DC)
     return;
 
   for (auto *D : (Deserialize ? DC->decls() : DC->noload_decls()))
     dumpDecl(D);
-
-  if (DC->hasExternalLexicalStorage()) {
-    dumpChild([=] {
-      ColorScope Color(OS, ShowColors, UndeserializedColor);
-      OS << "<undeserialized declarations>";
-    });
-  }
 }
 
 void ASTDumper::dumpLookups(const DeclContext *DC, bool DumpDecls) {
   dumpChild([=] {
     OS << "StoredDeclsMap ";
     NodeDumper.dumpBareDeclRef(cast<Decl>(DC));
 
     const DeclContext *Primary = DC->getPrimaryContext();
for (Decl::attr_iterator I = D->attr_begin(), E = D->attr_end(); I != E;
          ++I)
       dumpAttr(*I);
 
     if (const FullComment *Comment =
             D->getASTContext().getLocalCommentForDeclUncached(D))
       dumpComment(Comment, Comment);
 
     // Decls within functions are visited by the body.
     if (!isa<FunctionDecl>(*D) && !isa<ObjCMethodDecl>(*D)) {
       if (const auto *DC = dyn_cast<DeclContext>(D))
         dumpDeclContext(DC);
     }
   });
 }
 
 void ASTDumper::VisitLabelDecl(const LabelDecl *D) { NodeDumper.dumpName(D); }
 
 void ASTDumper::VisitTypedefDecl(const TypedefDecl *D) {
void ASTDumper::VisitObjCMethodDecl(const ObjCMethodDecl *D) {
   else
     OS << " +";
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getReturnType());
 
   if (D->isVariadic())
     OS << " variadic";
 
   if (D->isThisDeclarationADefinition())
     dumpDeclContext(D);
   else
     for (const ParmVarDecl *Parameter : D->parameters())
       dumpDecl(Parameter);
 
   if (D->hasBody())
     dumpStmt(D->getBody());
 }

lib/AST/TextNodeDumper.cpp

void TextNodeDumper::Visit(const Decl *D) {
   else if (D->isThisDeclarationReferenced())
     OS << " referenced";
 
   if (D->isInvalidDecl())
     OS << " invalid";
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
     if (FD->isConstexpr())
       OS << " constexpr";
+
+  if (!isa<FunctionDecl>(*D)) {
+    const auto *MD = dyn_cast<ObjCMethodDecl>(D);
+    if (!MD || !MD->isThisDeclarationADefinition()) {
+      const auto *DC = dyn_cast<DeclContext>(D);
+      if (DC && DC->hasExternalLexicalStorage()) {
+        ColorScope Color(OS, ShowColors, UndeserializedColor);
+        OS << " <undeserialized declarations>";
+      }
+    }
+  }
 }
 
 void TextNodeDumper::Visit(const CXXCtorInitializer *Init) {
   OS << "CXXCtorInitializer";
   if (Init->isAnyMemberInitializer()) {
     OS << ' ';
     dumpBareDeclRef(Init->getAnyMember());
   } else if (Init->isBaseInitializer()) {

test/AST/ast-dump-undeserialized.cpp

 // RUN: %clang_cc1 %s -chain-include %s -ast-dump | FileCheck -strict-whitespace %s
 
-// CHECK: TranslationUnitDecl 0x{{.+}} <<invalid sloc>> <invalid sloc>
-// CHECK: `-<undeserialized declarations>
+// CHECK: TranslationUnitDecl 0x{{.+}} <<invalid sloc>> <invalid sloc> <undeserialized declarations>