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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
