[PATCH] D28845: Prototype of modules codegen

David Blaikie via cfe-commits cfe-commits at lists.llvm.org
Sun Jan 29 21:12:39 PST 2017


On Sun, Jan 29, 2017 at 10:21 AM Richard Smith via Phabricator <
reviews at reviews.llvm.org> wrote:

> rsmith accepted this revision.
> rsmith added inline comments.
> This revision is now accepted and ready to land.
>
>
> ================
> Comment at: lib/AST/ExternalASTSource.cpp:33
> +ExternalASTSource::hasExternalDefinitions(unsigned ID) {
> +  return EK_ReplyHazy;
> +}
> ----------------
> dblaikie wrote:
> > 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?
> You could test this with `-chain-include`. I don't think there's any other
> in-tree way to get multiple external sources.
>

Ah - guess that doesn't make for very interesting testing, then - since I
wouldn't be able to make those external sources with interesting
differences in ExtDefs, I think? Happy to chat more if it's worth
testing/I've not understood how to exercise this.

- Dave


>
>
> https://reviews.llvm.org/D28845
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170130/58e589a0/attachment.html>


More information about the cfe-commits mailing list