[PATCH] D56829: Move decl context dumping to TextNodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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



================
Comment at: lib/AST/ASTDumper.cpp:506-507
     if (!isa<FunctionDecl>(*D) && !isa<ObjCMethodDecl>(*D)) {
       auto DC = dyn_cast<DeclContext>(D);
-      if (DC &&
-          (DC->hasExternalLexicalStorage() ||
-           (Deserialize ? DC->decls_begin() != DC->decls_end()
-                        : DC->noload_decls_begin() != DC->noload_decls_end())))
+      if (DC)
         dumpDeclContext(DC);
----------------
Might as well clean this up to be `if (const auto *DC = dyn_cast<DeclContext>(D))`


================
Comment at: lib/AST/ASTDumper.cpp:519-520
-          (DC->hasExternalLexicalStorage() ||
-           (Deserialize ? DC->decls_begin() != DC->decls_end()
-                        : DC->noload_decls_begin() != DC->noload_decls_end())))
         dumpDeclContext(DC);
----------------
Why did this condition get dropped?


================
Comment at: lib/AST/ASTDumper.cpp:1228
 
-  if (D->isThisDeclarationADefinition()) {
+  if (isa<DeclContext>(D) && D->isThisDeclarationADefinition())
     dumpDeclContext(D);
----------------
Why is the isa<> test needed? `D` must be an `ObjCMethodDecl` which inherits from `DeclContext`, so this seems unneeded.


================
Comment at: lib/AST/TextNodeDumper.cpp:261
+  if (!isa<FunctionDecl>(*D)) {
+    auto MD = dyn_cast<ObjCMethodDecl>(D);
+    if (!MD || !MD->isThisDeclarationADefinition()) {
----------------
`const auto *`


================
Comment at: lib/AST/TextNodeDumper.cpp:263
+    if (!MD || !MD->isThisDeclarationADefinition()) {
+      auto DC = dyn_cast<DeclContext>(D);
+      if (DC && DC->hasExternalLexicalStorage()) {
----------------
`const auto *`


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56829/new/

https://reviews.llvm.org/D56829