[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

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