Rename Minion to ASTImporterDelegate

[teemperor]

Summary

I think there universal agreement that Minion isn't the best name for this class. This patch renames the class
to ASTImporterDelegate to better reflect it's goal of monitoring and extending the ASTImporter.

[davide]

yay :)

[aprantl]

lldb/include/lldb/Symbol/ClangASTImporter.h

238

Is there some sort of doxygen comment we could add here?

[martong]

lldb/include/lldb/Symbol/ClangASTImporter.h

328

Would it make sense to rename the variables too? E.g. m_delegates?

[martong]

lldb/include/lldb/Symbol/ClangASTImporter.h

259

The name of the parameter is still minion ... if the goal is to completely wipe out the old name then perhaps these should be renamed. Also with the new type name it just feels odd.

[shafik]

This is a good change!

lldb/include/lldb/Symbol/ClangASTImporter.h

259

I fee like importer_delegate makes more sense now.

328

or m_importer_delegates

[teemperor]

• Added documentation.
• Fixed more references to 'minion' pointed out in the review.

lldb/include/lldb/Symbol/ClangASTImporter.h

struct DeclOrigin {
     bool Valid() { return (ctx != nullptr || decl != nullptr); }
 
     clang::ASTContext *ctx;
     clang::Decl *decl;
   };
 
   typedef std::map<const clang::Decl *, DeclOrigin> OriginMap;
 
-  class Minion : public clang::ASTImporter {
+  class ASTImporterDelegate : public clang::ASTImporter {
   public:
-    Minion(ClangASTImporter &master, clang::ASTContext *target_ctx,
+    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) {}
 
     /// Scope guard that attaches a CxxModuleHandler to a Minion and deattaches
     /// it at the end of the scope. Supports being used multiple times on the
-    /// same Minion instance in nested scopes.
+    /// same ASTImporterDelegate instance in nested scopes.
     class CxxModuleScope {
       /// The handler we attach to the Minion.
       CxxModuleHandler m_handler;
       /// The Minion we are supposed to attach the handler to.
-      Minion &m_minion;
+      ASTImporterDelegate &m_minion;
       /// True iff we attached the handler to the Minion.
       bool m_valid = false;
 
     public:
-      CxxModuleScope(Minion &minion, clang::ASTContext *dst_ctx)
+      CxxModuleScope(ASTImporterDelegate &minion, clang::ASTContext *dst_ctx)
           : m_minion(minion) {
         // If the minion doesn't have a CxxModuleHandler yet, create one
         // and attach it.
         if (!minion.m_std_handler) {
           m_handler = CxxModuleHandler(minion, dst_ctx);
           m_valid = true;
           minion.m_std_handler = &m_handler;
         }
public:
     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;
   };
 
-  typedef std::shared_ptr<Minion> MinionSP;
-  typedef std::map<clang::ASTContext *, MinionSP> MinionMap;
+  typedef std::shared_ptr<ASTImporterDelegate> ImporterDelegateSP;
+  typedef std::map<clang::ASTContext *, ImporterDelegateSP> DelegateMap;
   typedef std::map<const clang::NamespaceDecl *, NamespaceMapSP>
       NamespaceMetaMap;
 
   struct ASTContextMetadata {
     ASTContextMetadata(clang::ASTContext *dst_ctx)
         : m_dst_ctx(dst_ctx), m_minions(), m_origins(), m_namespace_maps(),
           m_map_completer(nullptr) {}
 
     clang::ASTContext *m_dst_ctx;
-    MinionMap m_minions;
+    DelegateMap m_minions;
     OriginMap m_origins;
 
     NamespaceMetaMap m_namespace_maps;
     MapCompleter *m_map_completer;
   };
 
   typedef std::shared_ptr<ASTContextMetadata> ASTContextMetadataSP;
   typedef std::map<const clang::ASTContext *, ASTContextMetadataSP>
ASTContextMetadataSP MaybeGetContextMetadata(clang::ASTContext *dst_ctx) {
     ContextMetadataMap::iterator context_md_iter = m_metadata_map.find(dst_ctx);
 
     if (context_md_iter != m_metadata_map.end())
       return context_md_iter->second;
     else
       return ASTContextMetadataSP();
   }
 
-  MinionSP GetMinion(clang::ASTContext *dst_ctx, clang::ASTContext *src_ctx) {
+  ImporterDelegateSP GetDelegate(clang::ASTContext *dst_ctx,
+                                 clang::ASTContext *src_ctx) {
     ASTContextMetadataSP context_md = GetContextMetadata(dst_ctx);
 
-    MinionMap &minions = context_md->m_minions;
-    MinionMap::iterator minion_iter = minions.find(src_ctx);
+    DelegateMap &minions = context_md->m_minions;
+    DelegateMap::iterator minion_iter = minions.find(src_ctx);
 
     if (minion_iter == minions.end()) {
-      MinionSP minion = MinionSP(new Minion(*this, dst_ctx, src_ctx));
+      ImporterDelegateSP minion =
+          ImporterDelegateSP(new ASTImporterDelegate(*this, dst_ctx, src_ctx));
       minions[src_ctx] = minion;
       return minion;
     } else {
       return minion_iter->second;
     }
   }
 
   DeclOrigin GetDeclOrigin(const clang::Decl *decl);

lldb/source/Symbol/ClangASTImporter.cpp

void ClangASTMetrics::DumpCounters(Log *log) {
   DumpCounters(log, global_counters);
   log->Printf("-- Local metrics --");
   DumpCounters(log, local_counters);
 }
 
 clang::QualType ClangASTImporter::CopyType(clang::ASTContext *dst_ast,
                                            clang::ASTContext *src_ast,
                                            clang::QualType type) {
-  MinionSP minion_sp(GetMinion(dst_ast, src_ast));
+  ImporterDelegateSP delegate_sp(GetDelegate(dst_ast, src_ast));
 
-  Minion::CxxModuleScope std_scope(*minion_sp, dst_ast);
+  ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (minion_sp)
-    return minion_sp->Import(type);
+  if (delegate_sp)
+    return delegate_sp->Import(type);
 
   return QualType();
 }
 
 lldb::opaque_compiler_type_t
 ClangASTImporter::CopyType(clang::ASTContext *dst_ast,
                            clang::ASTContext *src_ast,
                            lldb::opaque_compiler_type_t type) {
if (dst_clang_ast) {
     }
   }
   return CompilerType();
 }
 
 clang::Decl *ClangASTImporter::CopyDecl(clang::ASTContext *dst_ast,
                                         clang::ASTContext *src_ast,
                                         clang::Decl *decl) {
-  MinionSP minion_sp;
+  ImporterDelegateSP delegate_sp;
 
-  minion_sp = GetMinion(dst_ast, src_ast);
+  delegate_sp = GetDelegate(dst_ast, src_ast);
 
-  Minion::CxxModuleScope std_scope(*minion_sp, dst_ast);
+  ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (minion_sp) {
-    clang::Decl *result = minion_sp->Import(decl);
+  if (delegate_sp) {
+    clang::Decl *result = delegate_sp->Import(decl);
 
     if (!result) {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
       if (log) {
         lldb::user_id_t user_id = LLDB_INVALID_UID;
         ClangASTMetadata *metadata = GetDeclMetadata(decl);
         if (metadata)
ClangASTImporter::DeportType(clang::ASTContext *dst_ctx,
 
   if (log)
     log->Printf("    [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));
 
-  MinionSP minion_sp(GetMinion(dst_ctx, src_ctx));
+  ImporterDelegateSP delegate_sp(GetDelegate(dst_ctx, src_ctx));
 
-  if (!minion_sp)
+  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());
   }
 
-  minion_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported);
+  delegate_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported);
 
   lldb::opaque_compiler_type_t result = CopyType(dst_ctx, src_ctx, type);
 
-  minion_sp->ExecuteDeportWorkQueues();
+  delegate_sp->ExecuteDeportWorkQueues();
 
   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));
 
   if (log)
     log->Printf("    [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));
 
-  MinionSP minion_sp(GetMinion(dst_ctx, src_ctx));
+  ImporterDelegateSP delegate_sp(GetDelegate(dst_ctx, src_ctx));
 
-  if (!minion_sp)
+  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);
 
-  minion_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported);
+  delegate_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported);
 
   clang::Decl *result = CopyDecl(dst_ctx, src_ctx, decl);
 
-  minion_sp->ExecuteDeportWorkQueues();
+  delegate_sp->ExecuteDeportWorkQueues();
 
   if (!result)
     return nullptr;
 
   if (log)
     log->Printf(
         "    [ClangASTImporter] DeportDecl deported (%sDecl*)%p to (%sDecl*)%p",
         decl->getDeclKindName(), static_cast<void *>(decl),
bool ClangASTImporter::CompleteTagDecl(clang::TagDecl *decl) {
   DeclOrigin decl_origin = GetDeclOrigin(decl);
 
   if (!decl_origin.Valid())
     return false;
 
   if (!ClangASTContext::GetCompleteDecl(decl_origin.ctx, decl_origin.decl))
     return false;
 
-  MinionSP minion_sp(GetMinion(&decl->getASTContext(), decl_origin.ctx));
+  ImporterDelegateSP delegate_sp(
+      GetDelegate(&decl->getASTContext(), decl_origin.ctx));
 
-  Minion::CxxModuleScope std_scope(*minion_sp, &decl->getASTContext());
-  if (minion_sp)
-    minion_sp->ImportDefinitionTo(decl, decl_origin.decl);
+  ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp,
+                                                &decl->getASTContext());
+  if (delegate_sp)
+    delegate_sp->ImportDefinitionTo(decl, decl_origin.decl);
 
   return true;
 }
 
 bool ClangASTImporter::CompleteTagDeclWithOrigin(clang::TagDecl *decl,
                                                  clang::TagDecl *origin_decl) {
   ClangASTMetrics::RegisterDeclCompletion();
 
   clang::ASTContext *origin_ast_ctx = &origin_decl->getASTContext();
 
   if (!ClangASTContext::GetCompleteDecl(origin_ast_ctx, origin_decl))
     return false;
 
-  MinionSP minion_sp(GetMinion(&decl->getASTContext(), origin_ast_ctx));
+  ImporterDelegateSP delegate_sp(
+      GetDelegate(&decl->getASTContext(), origin_ast_ctx));
 
-  if (minion_sp)
-    minion_sp->ImportDefinitionTo(decl, origin_decl);
+  if (delegate_sp)
+    delegate_sp->ImportDefinitionTo(decl, origin_decl);
 
   ASTContextMetadataSP context_md = GetContextMetadata(&decl->getASTContext());
 
   OriginMap &origins = context_md->m_origins;
 
   origins[decl] = DeclOrigin(origin_ast_ctx, origin_decl);
 
   return true;
 }
 
 bool ClangASTImporter::CompleteObjCInterfaceDecl(
     clang::ObjCInterfaceDecl *interface_decl) {
   ClangASTMetrics::RegisterDeclCompletion();
 
   DeclOrigin decl_origin = GetDeclOrigin(interface_decl);
 
   if (!decl_origin.Valid())
     return false;
 
   if (!ClangASTContext::GetCompleteDecl(decl_origin.ctx, decl_origin.decl))
     return false;
 
-  MinionSP minion_sp(
-      GetMinion(&interface_decl->getASTContext(), decl_origin.ctx));
+  ImporterDelegateSP delegate_sp(
+      GetDelegate(&interface_decl->getASTContext(), decl_origin.ctx));
 
-  if (minion_sp)
-    minion_sp->ImportDefinitionTo(interface_decl, decl_origin.decl);
+  if (delegate_sp)
+    delegate_sp->ImportDefinitionTo(interface_decl, decl_origin.decl);
 
   if (ObjCInterfaceDecl *super_class = interface_decl->getSuperClass())
     RequireCompleteType(clang::QualType(super_class->getTypeForDecl(), 0));
 
   return true;
 }
 
 bool ClangASTImporter::CompleteAndFetchChildren(clang::QualType type) {
   if (!RequireCompleteType(type))
     return false;
 
   if (const TagType *tag_type = type->getAs<TagType>()) {
     TagDecl *tag_decl = tag_type->getDecl();
 
     DeclOrigin decl_origin = GetDeclOrigin(tag_decl);
 
     if (!decl_origin.Valid())
       return false;
 
-    MinionSP minion_sp(GetMinion(&tag_decl->getASTContext(), decl_origin.ctx));
+    ImporterDelegateSP delegate_sp(
+        GetDelegate(&tag_decl->getASTContext(), decl_origin.ctx));
 
-    Minion::CxxModuleScope std_scope(*minion_sp, &tag_decl->getASTContext());
+    ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp,
+                                                  &tag_decl->getASTContext());
 
     TagDecl *origin_tag_decl = llvm::dyn_cast<TagDecl>(decl_origin.decl);
 
     for (Decl *origin_child_decl : origin_tag_decl->decls()) {
-      minion_sp->Import(origin_child_decl);
+      delegate_sp->Import(origin_child_decl);
     }
 
     if (RecordDecl *record_decl = dyn_cast<RecordDecl>(origin_tag_decl)) {
       record_decl->setHasLoadedFieldsFromExternalStorage(true);
     }
 
     return true;
   }
 
   if (const ObjCObjectType *objc_object_type = type->getAs<ObjCObjectType>()) {
     if (ObjCInterfaceDecl *objc_interface_decl =
             objc_object_type->getInterface()) {
       DeclOrigin decl_origin = GetDeclOrigin(objc_interface_decl);
 
       if (!decl_origin.Valid())
         return false;
 
-      MinionSP minion_sp(
-          GetMinion(&objc_interface_decl->getASTContext(), decl_origin.ctx));
+      ImporterDelegateSP delegate_sp(
+          GetDelegate(&objc_interface_decl->getASTContext(), decl_origin.ctx));
 
       ObjCInterfaceDecl *origin_interface_decl =
           llvm::dyn_cast<ObjCInterfaceDecl>(decl_origin.decl);
 
       for (Decl *origin_child_decl : origin_interface_decl->decls()) {
-        minion_sp->Import(origin_child_decl);
+        delegate_sp->Import(origin_child_decl);
       }
 
       return true;
     } else {
       return false;
     }
   }
 
if (iter->second.ctx == src_ast)
       md->m_origins.erase(iter++);
     else
       ++iter;
   }
 }
 
 ClangASTImporter::MapCompleter::~MapCompleter() { return; }
 
-llvm::Expected<Decl *> ClangASTImporter::Minion::ImportImpl(Decl *From) {
+llvm::Expected<Decl *>
+ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
   if (m_std_handler) {
     llvm::Optional<Decl *> D = m_std_handler->Import(From);
     if (D) {
       // Make sure we don't use this decl later to map it back to it's original
       // decl. The decl the CxxModuleHandler created has nothing to do with
       // the one from debug info, and linking those two would just cause the
       // ASTImporter to try 'updating' the module decl with the minimal one from
       // the debug info.
       m_decls_to_ignore.insert(*D);
       return *D;
     }
   }
 
   return ASTImporter::ImportImpl(From);
 }
 
-void ClangASTImporter::Minion::InitDeportWorkQueues(
+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::Minion::ExecuteDeportWorkQueues() {
+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();
while (!m_decls_to_deport->empty()) {
 
     to_context_md->m_origins.erase(decl);
   }
 
   m_decls_to_deport = nullptr;
   m_decls_already_deported = nullptr;
 }
 
-void ClangASTImporter::Minion::ImportDefinitionTo(clang::Decl *to,
-                                                  clang::Decl *from) {
+void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
+    clang::Decl *to, clang::Decl *from) {
   ASTImporter::Imported(from, to);
 
   /*
   if (to_objc_interface)
       to_objc_interface->startDefinition();
 
   CXXRecordDecl *to_cxx_record = dyn_cast<CXXRecordDecl>(to);
 
do {
         to_objc_interface->startDefinition();
 
       to_objc_interface->setSuperClass(m_source_ctx->getTrivialTypeSourceInfo(
           m_source_ctx->getObjCInterfaceType(imported_from_superclass)));
     } while (0);
   }
 }
 
-void ClangASTImporter::Minion::Imported(clang::Decl *from,
+void ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl *from,
                                                      clang::Decl *to) {
   ClangASTMetrics::RegisterClangImport();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   // Some decls shouldn't be tracked here because they were not created by
   // copying 'from' to 'to'. Just exit early for those.
   if (m_decls_to_ignore.find(to) != m_decls_to_ignore.end())
     return clang::ASTImporter::Imported(from, to);
if (from_context_md) {
 
     if (origin_iter != origins.end()) {
       if (to_context_md->m_origins.find(to) == to_context_md->m_origins.end() ||
           user_id != LLDB_INVALID_UID) {
         if (origin_iter->second.ctx != &to->getASTContext())
           to_context_md->m_origins[to] = origin_iter->second;
       }
 
-      MinionSP direct_completer =
-          m_master.GetMinion(&to->getASTContext(), origin_iter->second.ctx);
+      ImporterDelegateSP direct_completer =
+          m_master.GetDelegate(&to->getASTContext(), origin_iter->second.ctx);
 
       if (direct_completer.get() != this)
         direct_completer->ASTImporter::Imported(origin_iter->second.decl, to);
 
       if (log)
         log->Printf("    [ClangASTImporter] Propagated origin "
                     "(Decl*)%p/(ASTContext*)%p from (ASTContext*)%p to "
                     "(ASTContext*)%p",
if (log) {
             ((Decl *)to_container_decl)->getDeclKindName(),
             (to_container_decl->hasExternalLexicalStorage() ? " Lexical" : ""),
             (to_container_decl->hasExternalVisibleStorage() ? " Visible" : ""));
       }
     }
   }
 }
 
-clang::Decl *ClangASTImporter::Minion::GetOriginalDecl(clang::Decl *To) {
+clang::Decl *
+ClangASTImporter::ASTImporterDelegate::GetOriginalDecl(clang::Decl *To) {
   ASTContextMetadataSP to_context_md =
       m_master.GetContextMetadata(&To->getASTContext());
 
   if (!to_context_md)
     return nullptr;
 
   OriginMap::iterator iter = to_context_md->m_origins.find(To);
 
   if (iter == to_context_md->m_origins.end())
     return nullptr;
 
   return const_cast<clang::Decl *>(iter->second.decl);
 }