[ASTPrinter] Simplify callbacks by dropping the notion of 'pending' callbacks

benlangmuir · benlangmuir · commit 5c29920acd5b · 2016-03-04T16:26:35.000-08:00
The three varieties of 'pending callbacks' made it hard to reason about
how callbacks would be presented to subclasses of ASTPrinter.  I
recently added new callbacks that would have made this even more complex
to deal with.

Luckily, it turns out they weren't buying us much, and simply forcing
any pending newlines and indentation to be printed ahead of making
callbacks was almost identical to the old behaviour. One complication is
that we now need to check for clang doc comments up front so we will
emit a newline in the right place.

This also incidentally fixed a bug in Loc vs Decl callback order.
diff --git a/include/swift/AST/ASTPrinter.h b/include/swift/AST/ASTPrinter.h
@@ -67,11 +67,6 @@ enum class PrintStructureKind {
 class ASTPrinter {
   unsigned CurrentIndentation = 0;
   unsigned PendingNewlines = 0;
-  /// The queue of pending printDeclPre callbacks that will be run when we print
-  /// a non-whitespace character.
-  SmallVector<const Decl *, 4> PendingDeclPreCallbacks;
-  const Decl *PendingDeclLocCallback = nullptr;
-  Optional<PrintNameContext> PendingNamePreCallback;
   const NominalTypeDecl *SynthesizeTarget = nullptr;
 
   void printTextImpl(StringRef Text);
@@ -192,60 +187,51 @@ class ASTPrinter {
     PendingNewlines++;
   }
 
+  void forceNewlines() {
+    if (PendingNewlines > 0) {
+      llvm::SmallString<16> Str;
+      for (unsigned i = 0; i != PendingNewlines; ++i)
+        Str += '\n';
+      PendingNewlines = 0;
+      printText(Str);
+      printIndent();
+    }
+  }
+
   virtual void printIndent();
 
   // MARK: Callback interface wrappers that perform ASTPrinter bookkeeping.
 
-  /// Schedule a \c printDeclPre callback to be called as soon as a
-  /// non-whitespace character is printed.
-  void callPrintDeclPre(const Decl *D) {
-    PendingDeclPreCallbacks.emplace_back(D);
-  }
+   /// Make a callback to printDeclPre(), performing any necessary bookeeping.
+  void callPrintDeclPre(const Decl *D);
 
   /// Make a callback to printDeclPost(), performing any necessary bookeeping.
   void callPrintDeclPost(const Decl *D) {
-    if (!PendingDeclPreCallbacks.empty() &&
-        PendingDeclPreCallbacks.back() == D) {
-      // Nothing printed for D; skip both pre and post callbacks.
-      // Ideally we wouldn't get as far as setting up the callback if we aren't
-      // going to print anything, but currently that would mean walking the
-      // children of top-level code decls to determine.
-      PendingDeclPreCallbacks.pop_back();
-      return;
-    }
     printDeclPost(D);
   }
 
   /// Make a callback to avoidPrintDeclPost(), performing any necessary
   /// bookkeeping.
   void callAvoidPrintDeclPost(const Decl *D) {
-    assert((PendingDeclPreCallbacks.empty() ||
-            PendingDeclPreCallbacks.back() != D) &&
-           "printDeclPre should not be called on avoided decl");
     avoidPrintDeclPost(D);
   }
 
-  /// Schedule a \c printDeclLoc callback to be called as soon as a
-  /// non-whitespace character is printed.
+   /// Make a callback to printDeclLoc(), performing any necessary bookeeping.
   void callPrintDeclLoc(const Decl *D) {
-    assert(!PendingDeclLocCallback && "unexpected nested callPrintDeclLoc");
-    PendingDeclLocCallback = D;
+    forceNewlines();
+    printDeclLoc(D);
   }
 
-  /// Schedule a \c printNamePre callback to be called as soon as a
-  /// non-whitespace character is printed.
+   /// Make a callback to printNamePre(), performing any necessary bookeeping.
   void callPrintNamePre(PrintNameContext Context) {
-    assert(!PendingNamePreCallback && "unexpected nested callPrintNamePre");
-    PendingNamePreCallback = Context;
+    forceNewlines();
+    printNamePre(Context);
   }
 
   /// Make a callback to printStructurePre(), performing any necessary
   /// bookkeeping.
   void callPrintStructurePre(PrintStructureKind Kind, const Decl *D = nullptr) {
-    // FIXME: As a hack, print an empty string to force the pending callbacks.
-    // The real fix is to incorporate the structure kinds into the pending
-    // callbacks, interleaved with the decls.
-    printTextImpl("");
+    forceNewlines();
     printStructurePre(Kind, D);
   }
 
diff --git a/lib/AST/ASTPrinter.cpp b/lib/AST/ASTPrinter.cpp
@@ -481,37 +481,7 @@ void ASTPrinter::printIndent() {
 }
 
 void ASTPrinter::printTextImpl(StringRef Text) {
-  if (PendingNewlines != 0) {
-    llvm::SmallString<16> Str;
-    for (unsigned i = 0; i != PendingNewlines; ++i)
-      Str += '\n';
-    PendingNewlines = 0;
-
-    printText(Str);
-    printIndent();
-  }
-
-  // Get the pending callbacks and remove them from the printer. They must all
-  // be removed before calling any of them to ensure correct ordering.
-  auto pendingDeclPre = PendingDeclPreCallbacks;
-  PendingDeclPreCallbacks.clear();
-  auto *LocD = PendingDeclLocCallback;
-  PendingDeclLocCallback = nullptr;
-  auto NameContext = PendingNamePreCallback;
-  PendingNamePreCallback.reset();
-
-  // Perform pending callbacks.
-  for (const Decl *PreD : pendingDeclPre) {
-    if (SynthesizeTarget && PreD->getKind() == DeclKind::Extension)
-      printSynthesizedExtensionPre(cast<ExtensionDecl>(PreD), SynthesizeTarget);
-    else
-      printDeclPre(PreD);
-  }
-  if (LocD)
-    printDeclLoc(LocD);
-  if (NameContext)
-    printNamePre(*NameContext);
-
+  forceNewlines();
   printText(Text);
 }
 
@@ -529,6 +499,15 @@ void ASTPrinter::printModuleRef(ModuleEntity Mod, Identifier Name) {
   printName(Name);
 }
 
+void ASTPrinter::callPrintDeclPre(const Decl *D) {
+  forceNewlines();
+
+  if (SynthesizeTarget && D->getKind() == DeclKind::Extension)
+    printSynthesizedExtensionPre(cast<ExtensionDecl>(D), SynthesizeTarget);
+  else
+    printDeclPre(D);
+}
+
 ASTPrinter &ASTPrinter::operator<<(unsigned long long N) {
   llvm::SmallString<32> Str;
   llvm::raw_svector_ostream OS(Str);
@@ -675,11 +654,6 @@ class PrintAST : public ASTVisitor<PrintAST> {
     if (!RC)
       return;
 
-    if (!Options.PrintRegularClangComments) {
-      Printer.printNewline();
-      indent();
-    }
-
     bool Invalid;
     unsigned StartLocCol =
         ClangContext.getSourceManager().getSpellingColumnNumber(
@@ -882,6 +856,23 @@ class PrintAST : public ASTVisitor<PrintAST> {
                       D->getKind() == DeclKind::Extension;
     if (Synthesize)
       Printer.setSynthesizedTarget(Options.TransformContext->getNominal());
+
+    // We want to print a newline before doc comments.  Swift code already
+    // handles this, but we need to insert it for clang doc comments when not
+    // printing other clang comments. Do it now so the printDeclPre callback
+    // happens after the newline.
+    if (Options.PrintDocumentationComments &&
+        !Options.PrintRegularClangComments &&
+        D->hasClangNode()) {
+      auto clangNode = D->getClangNode();
+      auto clangDecl = clangNode.getAsDecl();
+      if (clangDecl &&
+          clangDecl->getASTContext().getRawCommentForAnyRedecl(clangDecl)) {
+        Printer.printNewline();
+        indent();
+      }
+    }
+
     Printer.callPrintDeclPre(D);
     ASTVisitor::visit(D);
     if (Synthesize) {
diff --git a/test/IDE/Inputs/mock-sdk/Foo.annotated.txt b/test/IDE/Inputs/mock-sdk/Foo.annotated.txt
@@ -195,17 +195,17 @@ class <loc>FooClassDerived</loc> : <ref:Class>FooClassBase</ref>, <ref:Protocol>
   <decl:Constructor><loc>init()</loc></decl>
   <decl:Constructor><loc>init(<decl:Param>x: <ref:Struct>Int32</ref></decl>)</loc></decl>
 }</decl>
-<decl:Extension>extension <ref:Class><loc>FooClassBase</ref></loc> {
+<decl:Extension>extension <loc><ref:Class>FooClassBase</ref></loc> {
   <decl:Func>class func <loc>_internalMeth1()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
   <decl:Func>func <loc>_internalMeth1()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
 }</decl>
-<decl:Extension>extension <ref:Class><loc>FooClassBase</ref></loc> {
+<decl:Extension>extension <loc><ref:Class>FooClassBase</ref></loc> {
   <decl:Func>class func <loc>_internalMeth2()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
   <decl:Func>func <loc>_internalMeth2()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
   <decl:Func>class func <loc>nonInternalMeth()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
   <decl:Func>func <loc>nonInternalMeth()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
 }</decl>
-<decl:Extension>extension <ref:Class><loc>FooClassBase</ref></loc> {
+<decl:Extension>extension <loc><ref:Class>FooClassBase</ref></loc> {
   <decl:Func>class func <loc>_internalMeth3()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
   <decl:Func>func <loc>_internalMeth3()</loc> -> <ref:Protocol>AnyObject</ref>!</decl>
 }</decl>
@@ -265,11 +265,11 @@ func <loc>FooCFTypeRelease(<decl:Param>_: <ref:Class>FooCFType</ref>!</decl>)</l
   <decl:Constructor><loc>init!()</loc></decl>
   <decl:Constructor>convenience <loc>init!(<decl:Param>float f: <ref:Struct>Float</ref></decl>)</loc></decl>
 }</decl>
-<decl:Extension>extension <ref:Class><loc>FooRepeatedMembers</ref></loc> {
+<decl:Extension>extension <loc><ref:Class>FooRepeatedMembers</ref></loc> {
   <decl:Var>var <loc>repeatedPropertyInCategory</loc>: <ref:Struct>Int32</ref></decl>
   <decl:Func>func <loc>repeatedMethodInCategory()</loc></decl>
 }</decl>
-<decl:Extension>extension <ref:Class><loc>FooRepeatedMembers</ref></loc> {
+<decl:Extension>extension <loc><ref:Class>FooRepeatedMembers</ref></loc> {
   <decl:Var>var <loc>repeatedPropertyFromCategory</loc>: <ref:Struct>Int32</ref></decl>
   <decl:Func>func <loc>repeatedMethodFromCategory()</loc></decl>
 }</decl>
