[PATCH] D28845: Prototype of modules codegen

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.





================
Comment at: include/clang/AST/ASTContext.h:2490
   /// it is not used.
-  bool DeclMustBeEmitted(const Decl *D);
+  bool DeclMustBeEmitted(const Decl *D, bool WritingModule = false);
 
----------------
I think the name of this flag might be out of sync with its meaning; it looks like it means "must this declaration be emitted when performing modular codegen?"


================
Comment at: include/clang/Basic/Module.h:208
 
+  unsigned WithCodegen : 2;
+
----------------
Does this need to be two bits wide? Seems to only store true/false.


================
Comment at: include/clang/Driver/CC1Options.td:435-438
+def fmodule_codegen :
+  Flag<["-"], "fmodule-codegen">,
+  HelpText<"Generate code for uses of this module that assumes an explicit "
+           "object file will be built for the module">;
----------------
Maybe `module` -> `modules`, to match the other `-fmodules-*` flags controlling various options.


================
Comment at: lib/AST/ASTContext.cpp:9020-9023
+      if (auto *Ext = getExternalSource())
+        if (Ext->hasExternalDefinitions(FD->getOwningModuleID()) ==
+            ExternalASTSource::EK_Never)
+          return true;
----------------
I think this `return true` is unreachable and can be deleted: if we get here with `Linkage == GVA_DiscardableODR`, then the call to `hasExternalDefinitions` in `GetGVALinkageForFunction` must have returned `EK_ReplyHazy`. (In the case this is checking for, `GetGVALinkageForFunction` would return `GVA_StrongODR`, so the check below will return `true` anyway.)


================
Comment at: lib/AST/ASTContext.cpp:9029
     // Implicit template instantiations can also be deferred in C++.
     return !isDiscardableGVALinkage(GetGVALinkageForFunction(FD));
   }
----------------
Pass `Linkage` in here instead of recomputing it


================
Comment at: lib/AST/ExternalASTSource.cpp:33
+ExternalASTSource::hasExternalDefinitions(unsigned ID) {
+  return EK_ReplyHazy;
+}
----------------
You should add support for this function to `clang::MultiplexExternalSemaSource` too.


================
Comment at: lib/Serialization/ASTWriterDecl.cpp:2215-2219
+  if (isRequiredDecl(D, Context, WritingModule, false))
     EagerlyDeserializedDecls.push_back(ID);
+  else if (Context.getLangOpts().ModularCodegen && WritingModule &&
+           isRequiredDecl(D, Context, true, true))
+    ModularCodegenDecls.push_back(ID);
----------------
I suspect we'll want to seriously prune back the contents of `EagerlyDeserializedDecls` for the modular codegen case at some point, but we don't need to do that in this change.


https://reviews.llvm.org/D28845