[PATCH] D28845: Prototype of modules codegen

[David Blaikie via Phabricator via cfe-commits]

dblaikie marked 6 inline comments as done.
dblaikie added a comment.

Addressed CR feedback



================
Comment at: lib/AST/ExternalASTSource.cpp:33
+ExternalASTSource::hasExternalDefinitions(unsigned ID) {
+  return EK_ReplyHazy;
+}
----------------
rsmith wrote:
> You should add support for this function to `clang::MultiplexExternalSemaSource` too.
Done - though is there any test coverage I should add? Not sure exactly when/where/how this is used.

Also only made a rough guess at what the right behavior is here. It could be a bit more obvious if I made "hasExternalDefinitions" return Optional<ExtKind> - then when we find an ExternalASTSource that owns the ID (are the IDs unique across the MultiplexExternalSemaSource? I assume they have to be, but thought they'd only be valid within a single Module... guess I'm confused in a few ways - certainly haven't thought about it in great detail) we'd know to stop asking other sources. Probably not worth it unless there's a lot of sources?


================
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);
----------------
rsmith wrote:
> 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.
Right - I was wondering if we'd end up with anything in EagerlyDeserializedDecls when modular codegen is fully implemented. (if that's the case - we could have only one list, and a 'bit' to say whether it's modular codegen decls or EagerlyDeserializedDecls, if these records/blobs/whatnot are expensive to have two of rather than one - but I don't think that's the case so probably more readable/self-documenting to use two lists as we are)


https://reviews.llvm.org/D28845