Move decl completion out of the ASTImporterDelegate and document it [NFC]

[teemperor]

Summary

The ASTImporterDelegate is currently responsible for both recording and also completing
types. This patch moves the actual completion and recording code outside the ASTImporterDelegate
to reduce the amount of responsibilities the ASTImporterDelegate has to fulfill.

As I anyway had to touch the code when moving I also documented and refactored most of it
(e.g. no more asserts that we call the deporting start/end function always as a pair).

Note that I had to make the ASTImporterDelegate and it's related functions public now so that
I can move out the functionality in another class (that doesn't need to be in the header).

[martong]

This is a good patch, it is good to separate responsibilities and it makes cleaner how the clang::ASTImporter is used.

lldb/source/Symbol/ClangASTImporter.cpp

250 ↗

(On Diff #197920)

The verb deport is pretty vague in this context for me. Actually, what this class does is importing missing members and methods of classes. Perhaps a better name could be ImportMembersQueueScope ?
I still don't understand why is it needed to import the members in two steps in LLDB: 1. import the class itself without members 2. import the members. So perhaps we could have some documentation about that too here.

324 ↗

(On Diff #197920)

Would it make sense to filter out those TagDecls which are already completed?
E.g.:

if (TagDecl *to_tag_decl = dyn_cast<TagDecl>(to))
  if (to_tag_decl->isCompleteDefinition()) // skip tags which are already completed
    return;

Or this would not work because there are cases when the tag is completed, but the members are still missing? If that is the case could you please document that here too?

[teemperor]

• Renamed class and related variables to reflect that it is completing TagDecls.

[teemperor]

lldb/source/Symbol/ClangASTImporter.cpp

250 ↗

(On Diff #197920)

Renamed to point out that it's essentially just completing TagDecls (I didn't want to specify the members, that seems a bit vague).

And I wish I could document why we need to do this in two steps, but I didn't write that code so that's also a mystery to me :) When I'll figure this out I'll make a patch.

324 ↗

(On Diff #197920)

Maybe, but that could make this patch non-NFC :) I can make this as a follow-up.

[martong]

Looks good to me, thanks!

lldb/source/Symbol/ClangASTImporter.cpp

328 ↗

(On Diff #220965)

typo: "to completed" -> "to complete"

324 ↗

(On Diff #197920)

Okay.

lldb/trunk/include/lldb/Symbol/ClangASTImporter.h

void InstallMapCompleter(clang::ASTContext *dst_ctx,
     }
 
     context_md->m_map_completer = &completer;
   }
 
   void ForgetDestination(clang::ASTContext *dst_ctx);
   void ForgetSource(clang::ASTContext *dst_ctx, clang::ASTContext *src_ctx);
 
-private:
+public:
   struct DeclOrigin {
     DeclOrigin() : ctx(nullptr), decl(nullptr) {}
 
     DeclOrigin(clang::ASTContext *_ctx, clang::Decl *_decl)
         : ctx(_ctx), decl(_decl) {}
 
     DeclOrigin(const DeclOrigin &rhs) {
       ctx = rhs.ctx;
       decl = rhs.decl;
     }
 
     void operator=(const DeclOrigin &rhs) {
       ctx = rhs.ctx;
       decl = rhs.decl;
     }
 
     bool Valid() { return (ctx != nullptr || decl != nullptr); }
 
     clang::ASTContext *ctx;
     clang::Decl *decl;
   };
 
   typedef std::map<const clang::Decl *, DeclOrigin> OriginMap;
 
+  /// Listener interface used by the ASTImporterDelegate to inform other code
+  /// about decls that have been imported the first time.
+  struct NewDeclListener {
+    virtual ~NewDeclListener() = default;
+    /// A decl has been imported for the first time.
+    virtual void NewDeclImported(clang::Decl *from, clang::Decl *to) = 0;
+  };
+
   /// ASTImporter that intercepts and records the import process of the
   /// underlying ASTImporter.
   ///
   /// This class updates the map from declarations to their original
-  /// declarations and can record and complete declarations that have been
-  /// imported in a certain interval.
+  /// declarations and can record declarations that have been imported in a
+  /// certain interval.
   ///
   /// When intercepting a declaration import, the ASTImporterDelegate uses the
   /// CxxModuleHandler to replace any missing or malformed declarations with
   /// their counterpart from a C++ module.
-  class ASTImporterDelegate : public clang::ASTImporter {
-  public:
+  struct ASTImporterDelegate : public clang::ASTImporter {
     ASTImporterDelegate(ClangASTImporter &master, clang::ASTContext *target_ctx,
                         clang::ASTContext *source_ctx)
         : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
                              master.m_file_manager, true /*minimal*/),
-          m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
           m_master(master), m_source_ctx(source_ctx) {
       setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
     }
 
     /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
     /// and deattaches it at the end of the scope. Supports being used multiple
     /// times on the same ASTImporterDelegate instance in nested scopes.
     class CxxModuleScope {
public:
         if (m_valid) {
           // Make sure no one messed with the handler we placed.
           assert(m_delegate.m_std_handler == &m_handler);
           m_delegate.m_std_handler = nullptr;
         }
       }
     };
 
-  protected:
-    llvm::Expected<clang::Decl *> ImportImpl(clang::Decl *From) override;
-
-  public:
-    // A call to "InitDeportWorkQueues" puts the delegate into deport mode.
-    // In deport mode, every copied Decl that could require completion is
-    // recorded and placed into the decls_to_deport set.
-    //
-    // A call to "ExecuteDeportWorkQueues" completes all the Decls that
-    // are in decls_to_deport, adding any Decls it sees along the way that it
-    // hasn't already deported.  It proceeds until decls_to_deport is empty.
-    //
-    // These calls must be paired.  Leaving a delegate in deport mode or trying
-    // to start deport delegate with a new pair of queues will result in an
-    // assertion failure.
-
-    void
-    InitDeportWorkQueues(std::set<clang::NamedDecl *> *decls_to_deport,
-                         std::set<clang::NamedDecl *> *decls_already_deported);
-    void ExecuteDeportWorkQueues();
-
     void ImportDefinitionTo(clang::Decl *to, clang::Decl *from);
 
     void Imported(clang::Decl *from, clang::Decl *to) override;
 
     clang::Decl *GetOriginalDecl(clang::Decl *To) override;
 
+    void SetImportListener(NewDeclListener *listener) {
+      assert(m_new_decl_listener == nullptr && "Already attached a listener?");
+      m_new_decl_listener = listener;
+    }
+    void RemoveImportListener() { m_new_decl_listener = nullptr; }
+
+  protected:
+    llvm::Expected<clang::Decl *> ImportImpl(clang::Decl *From) override;
+
+  private:
     /// Decls we should ignore when mapping decls back to their original
     /// ASTContext. Used by the CxxModuleHandler to mark declarations that
     /// were created from the 'std' C++ module to prevent that the Importer
     /// tries to sync them with the broken equivalent in the debug info AST.
     std::set<clang::Decl *> m_decls_to_ignore;
-    std::set<clang::NamedDecl *> *m_decls_to_deport;
-    std::set<clang::NamedDecl *> *m_decls_already_deported;
     ClangASTImporter &m_master;
     clang::ASTContext *m_source_ctx;
     CxxModuleHandler *m_std_handler = nullptr;
+    /// The currently attached listener.
+    NewDeclListener *m_new_decl_listener = nullptr;
   };
 
   typedef std::shared_ptr<ASTImporterDelegate> ImporterDelegateSP;
   typedef std::map<clang::ASTContext *, ImporterDelegateSP> DelegateMap;
   typedef std::map<const clang::NamespaceDecl *, NamespaceMapSP>
       NamespaceMetaMap;
 
   struct ASTContextMetadata {
if (delegate_iter == delegates.end()) {
           ImporterDelegateSP(new ASTImporterDelegate(*this, dst_ctx, src_ctx));
       delegates[src_ctx] = delegate;
       return delegate;
     } else {
       return delegate_iter->second;
     }
   }
 
+public:
   DeclOrigin GetDeclOrigin(const clang::Decl *decl);
 
   clang::FileManager m_file_manager;
   typedef llvm::DenseMap<const clang::RecordDecl *, LayoutInfo>
       RecordDeclToLayoutMap;
 
   RecordDeclToLayoutMap m_record_decl_to_layout_map;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_ClangASTImporter_h_

lldb/trunk/source/Symbol/ClangASTImporter.cpp

public:
   ~DeclContextOverride() {
     for (const std::pair<clang::Decl *, Backup> &backup : m_backups) {
       backup.first->setDeclContext(backup.second.decl_context);
       backup.first->setLexicalDeclContext(backup.second.lexical_decl_context);
     }
   }
 };
 
+namespace {
+/// Completes all imported TagDecls at the end of the scope.
+///
+/// While in a CompleteTagDeclsScope, every decl that could be completed will
+/// be completed at the end of the scope (including all Decls that are
+/// imported while completing the original Decls).
+class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
+  ClangASTImporter::ImporterDelegateSP m_delegate;
+  // FIXME: Investigate how many decls we usually have in these sets and
+  // see if we can use SmallPtrSet instead here.
+  std::set<NamedDecl *> m_decls_to_complete;
+  std::set<NamedDecl *> m_decls_already_completed;
+  clang::ASTContext *m_dst_ctx;
+  clang::ASTContext *m_src_ctx;
+  ClangASTImporter &importer;
+
+public:
+  /// Constructs a CompleteTagDeclsScope.
+  /// \param importer The ClangASTImporter that we should observe.
+  /// \param dst_ctx The ASTContext to which Decls are imported.
+  /// \param src_ctx The ASTContext from which Decls are imported.
+  explicit CompleteTagDeclsScope(ClangASTImporter &importer,
+                            clang::ASTContext *dst_ctx,
+                            clang::ASTContext *src_ctx)
+      : m_delegate(importer.GetDelegate(dst_ctx, src_ctx)), m_dst_ctx(dst_ctx),
+        m_src_ctx(src_ctx), importer(importer) {
+    m_delegate->SetImportListener(this);
+  }
+
+  virtual ~CompleteTagDeclsScope() {
+    ClangASTImporter::ASTContextMetadataSP to_context_md =
+        importer.GetContextMetadata(m_dst_ctx);
+
+    // Complete all decls we collected until now.
+    while (!m_decls_to_complete.empty()) {
+      NamedDecl *decl = *m_decls_to_complete.begin();
+
+      m_decls_already_completed.insert(decl);
+      m_decls_to_complete.erase(decl);
+
+      // We should only complete decls coming from the source context.
+      assert(to_context_md->m_origins[decl].ctx == m_src_ctx);
+
+      Decl *original_decl = to_context_md->m_origins[decl].decl;
+
+      // Complete the decl now.
+      ClangASTContext::GetCompleteDecl(m_src_ctx, original_decl);
+      if (auto *tag_decl = dyn_cast<TagDecl>(decl)) {
+        if (auto *original_tag_decl = dyn_cast<TagDecl>(original_decl)) {
+          if (original_tag_decl->isCompleteDefinition()) {
+            m_delegate->ImportDefinitionTo(tag_decl, original_tag_decl);
+            tag_decl->setCompleteDefinition(true);
+          }
+        }
+
+        tag_decl->setHasExternalLexicalStorage(false);
+        tag_decl->setHasExternalVisibleStorage(false);
+      } else if (auto *container_decl = dyn_cast<ObjCContainerDecl>(decl)) {
+        container_decl->setHasExternalLexicalStorage(false);
+        container_decl->setHasExternalVisibleStorage(false);
+      }
+
+      to_context_md->m_origins.erase(decl);
+    }
+
+    // Stop listening to imported decls. We do this after clearing the
+    // Decls we needed to import to catch all Decls they might have pulled in.
+    m_delegate->RemoveImportListener();
+  }
+
+  void NewDeclImported(clang::Decl *from, clang::Decl *to) override {
+    // Filter out decls that we can't complete later.
+    if (!isa<TagDecl>(to) && !isa<ObjCInterfaceDecl>(to))
+      return;
+    RecordDecl *from_record_decl = dyn_cast<RecordDecl>(from);
+    // We don't need to complete injected class name decls.
+    if (from_record_decl && from_record_decl->isInjectedClassName())
+      return;
+
+    NamedDecl *to_named_decl = dyn_cast<NamedDecl>(to);
+    // Check if we already completed this type.
+    if (m_decls_already_completed.count(to_named_decl) != 0)
+      return;
+    m_decls_to_complete.insert(to_named_decl);
+  }
+};
+} // namespace
+
 lldb::opaque_compiler_type_t
 ClangASTImporter::DeportType(clang::ASTContext *dst_ctx,
                              clang::ASTContext *src_ctx,
                              lldb::opaque_compiler_type_t type) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   LLDB_LOGF(log,
             "    [ClangASTImporter] DeportType called on (%sType*)0x%llx "
             "from (ASTContext*)%p to (ASTContext*)%p",
             QualType::getFromOpaquePtr(type)->getTypeClassName(),
             (unsigned long long)type, static_cast<void *>(src_ctx),
             static_cast<void *>(dst_ctx));
 
-  ImporterDelegateSP delegate_sp(GetDelegate(dst_ctx, src_ctx));
-
-  if (!delegate_sp)
-    return nullptr;
-
-  std::set<NamedDecl *> decls_to_deport;
-  std::set<NamedDecl *> decls_already_deported;
-
   DeclContextOverride decl_context_override;
 
-  if (const clang::TagType *tag_type =
-          clang::QualType::getFromOpaquePtr(type)->getAs<TagType>()) {
-    decl_context_override.OverrideAllDeclsFromContainingFunction(
-        tag_type->getDecl());
-  }
-
-  delegate_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported);
-
-  lldb::opaque_compiler_type_t result = CopyType(dst_ctx, src_ctx, type);
+  if (auto *t = QualType::getFromOpaquePtr(type)->getAs<TagType>())
+    decl_context_override.OverrideAllDeclsFromContainingFunction(t->getDecl());
 
-  delegate_sp->ExecuteDeportWorkQueues();
+  lldb::opaque_compiler_type_t result;
+  {
+    CompleteTagDeclsScope complete_scope(*this, dst_ctx, src_ctx);
+    result = CopyType(dst_ctx, src_ctx, type);
+  }
 
   if (!result)
     return nullptr;
 
   return result;
 }
 
 clang::Decl *ClangASTImporter::DeportDecl(clang::ASTContext *dst_ctx,
                                           clang::ASTContext *src_ctx,
                                           clang::Decl *decl) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   LLDB_LOGF(log,
             "    [ClangASTImporter] DeportDecl called on (%sDecl*)%p from "
             "(ASTContext*)%p to (ASTContext*)%p",
             decl->getDeclKindName(), static_cast<void *>(decl),
             static_cast<void *>(src_ctx), static_cast<void *>(dst_ctx));
 
-  ImporterDelegateSP delegate_sp(GetDelegate(dst_ctx, src_ctx));
-
-  if (!delegate_sp)
-    return nullptr;
-
-  std::set<NamedDecl *> decls_to_deport;
-  std::set<NamedDecl *> decls_already_deported;
-
   DeclContextOverride decl_context_override;
 
   decl_context_override.OverrideAllDeclsFromContainingFunction(decl);
 
-  delegate_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported);
-
-  clang::Decl *result = CopyDecl(dst_ctx, src_ctx, decl);
-
-  delegate_sp->ExecuteDeportWorkQueues();
+  clang::Decl *result;
+  {
+    CompleteTagDeclsScope complete_scope(*this, dst_ctx, src_ctx);
+    result = CopyDecl(dst_ctx, src_ctx, decl);
+  }
 
   if (!result)
     return nullptr;
 
   LLDB_LOGF(
       log,
       "    [ClangASTImporter] DeportDecl deported (%sDecl*)%p to (%sDecl*)%p",
       decl->getDeclKindName(), static_cast<void *>(decl),
if (D) {
       m_decls_to_ignore.insert(*D);
       return *D;
     }
   }
 
   return ASTImporter::ImportImpl(From);
 }
 
-void ClangASTImporter::ASTImporterDelegate::InitDeportWorkQueues(
-    std::set<clang::NamedDecl *> *decls_to_deport,
-    std::set<clang::NamedDecl *> *decls_already_deported) {
-  assert(!m_decls_to_deport);
-  assert(!m_decls_already_deported);
-
-  m_decls_to_deport = decls_to_deport;
-  m_decls_already_deported = decls_already_deported;
-}
-
-void ClangASTImporter::ASTImporterDelegate::ExecuteDeportWorkQueues() {
-  assert(m_decls_to_deport);
-  assert(m_decls_already_deported);
-
-  ASTContextMetadataSP to_context_md =
-      m_master.GetContextMetadata(&getToContext());
-
-  while (!m_decls_to_deport->empty()) {
-    NamedDecl *decl = *m_decls_to_deport->begin();
-
-    m_decls_already_deported->insert(decl);
-    m_decls_to_deport->erase(decl);
-
-    DeclOrigin &origin = to_context_md->m_origins[decl];
-    UNUSED_IF_ASSERT_DISABLED(origin);
-
-    assert(origin.ctx ==
-           m_source_ctx); // otherwise we should never have added this
-                          // because it doesn't need to be deported
-
-    Decl *original_decl = to_context_md->m_origins[decl].decl;
-
-    ClangASTContext::GetCompleteDecl(m_source_ctx, original_decl);
-
-    if (TagDecl *tag_decl = dyn_cast<TagDecl>(decl)) {
-      if (TagDecl *original_tag_decl = dyn_cast<TagDecl>(original_decl)) {
-        if (original_tag_decl->isCompleteDefinition()) {
-          ImportDefinitionTo(tag_decl, original_tag_decl);
-          tag_decl->setCompleteDefinition(true);
-        }
-      }
-
-      tag_decl->setHasExternalLexicalStorage(false);
-      tag_decl->setHasExternalVisibleStorage(false);
-    } else if (ObjCContainerDecl *container_decl =
-                   dyn_cast<ObjCContainerDecl>(decl)) {
-      container_decl->setHasExternalLexicalStorage(false);
-      container_decl->setHasExternalVisibleStorage(false);
-    }
-
-    to_context_md->m_origins.erase(decl);
-  }
-
-  m_decls_to_deport = nullptr;
-  m_decls_already_deported = nullptr;
-}
-
 void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
     clang::Decl *to, clang::Decl *from) {
   ASTImporter::Imported(from, to);
 
   /*
   if (to_objc_interface)
       to_objc_interface->startDefinition();
 
if (origin_iter != origins.end()) {
                 "    [ClangASTImporter] Propagated origin "
                 "(Decl*)%p/(ASTContext*)%p from (ASTContext*)%p to "
                 "(ASTContext*)%p",
                 static_cast<void *>(origin_iter->second.decl),
                 static_cast<void *>(origin_iter->second.ctx),
                 static_cast<void *>(&from->getASTContext()),
                 static_cast<void *>(&to->getASTContext()));
     } else {
-      if (m_decls_to_deport && m_decls_already_deported) {
-        if (isa<TagDecl>(to) || isa<ObjCInterfaceDecl>(to)) {
-          RecordDecl *from_record_decl = dyn_cast<RecordDecl>(from);
-          if (from_record_decl == nullptr ||
-              !from_record_decl->isInjectedClassName()) {
-            NamedDecl *to_named_decl = dyn_cast<NamedDecl>(to);
-
-            if (!m_decls_already_deported->count(to_named_decl))
-              m_decls_to_deport->insert(to_named_decl);
-          }
-        }
-      }
+      if (m_new_decl_listener)
+        m_new_decl_listener->NewDeclImported(from, to);
 
       if (to_context_md->m_origins.find(to) == to_context_md->m_origins.end() ||
           user_id != LLDB_INVALID_UID) {
         to_context_md->m_origins[to] = DeclOrigin(m_source_ctx, from);
       }
 
       LLDB_LOGF(log,
                 "    [ClangASTImporter] Decl has no origin information in "