boris added inline comments.
================
Comment at: lib/Serialization/ASTReader.cpp:4145-4146
+ // by-name lookup.
+ if (Type == MK_PrebuiltModule || Type == MK_ExplicitModule)
+ ModuleMgr.registerPrebuilt(F);
break;
----------------
rsmith wrote:
> I'm worried by this: the `Type` can be different for different imports of the
> same .pcm file, and you'll only get here for the *first* import edge. So you
> can fail to add the module to the "prebuilt modules" map here, and then later
> attempt to look it up there (when processing the module offset map) and fail
> to find it.
>
> Instead of keeping a separate lookup structure on the module manager, could
> you look up the `Module`, and then use its `ASTFile` to find the
> corresponding `ModuleFile`?
>
> (If you go that way, the changes to module lookup when reading the module
> offset map could be generalized to apply to all `MK_*Module` types.)
Yes, I was not entirely happy with this register prebuilt business so thanks
for the hint. I've implemented this and all tests pass (both Clang's and my
own). Though I cannot say I fully understand all the possible scenarios (like
when can ASTFile be NULL).
> (If you go that way, the changes to module lookup when reading the module
> offset map could be generalized to apply to all MK_*Module types.)
Are you suggesting that we switch to storing module names instead of module
files for all the module types in the offset map? If so, I think I've tried
that at some point but it didn't go well (existing tests were failing if I
remember correctly). I can try again if you think this should work.
https://reviews.llvm.org/D35020
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
