On Fri, Mar 28, 2014 at 1:12 PM, Ben Langmuir <[email protected]> wrote:
> This patch allows multiple modules that have the same name to coexist in > the module cache. To differentiate between two modules with the same name, > we will consider the path the module map file that they are defined by* > part of the 'key' for looking up the precompiled module (pcm file). > Specifically, this patch renames the precompiled module (pcm) files from > > cache-path/<module hash>/Foo.pcm > > to > > cache-path/<module hash>/Foo-<hash of module map path>.pcm > >From a high level, I don't really see why we need a second hash here. Shouldn't the -I options be included in the <module hash>? If I build the same module with different -I flags, that should resolve to different .pcm files, regardless of whether it makes the module name resolve to a different module.map file. Are you trying to cope with the case where the -I path finds multiple module.map files defining the same module (where it's basically chance which one will get built and used)? I don't really feel like this is the right solution to that problem either -- we should remove the 'luck' aspect and use some sane mechanism to determine which module.map files are loaded, and in what order. Is this addressing some other case? > In addition, I've taught the ASTReader to re-resolve the names of imported > modules during module loading so that if the header search context changes > between when a module was originally built and when it is loaded we can > rebuild it if necessary. For example, if module A imports module B > > first time: > clang -I /path/to/A -I /path/to/B ... > > second time: > clang -I /path/to/A -I /different/path/to/B ... > > will now rebuild A as expected. > > > * in the case of inferred modules, we use the module map file that > *allowed* the inference, not the __inferred_module.map file, since the > inferred file path is the same for every inferred module. Review comments on the patch itself: + /// the inferrence (e.g. contained 'module *') rather than the virtual Typo 'inference', 'Module *'. + /// For an explanaition of \p ModuleMap, see Module::ModuleMap. Typo 'explanation'. + // safe becuase the FileManager is shared between the compiler instances. Typo 'because' + // the inferred module. If this->ModuleMap is nullptr, then we are using + // -emit-module directly, and we cannot have an inferred module. I don't understand what this comment is trying to say. If we're using -emit-module, then we were given a module map on the command-line; should that not be referred to by this->ModuleMap? (Also, why 'this->'?) How can a top-level module be inferred? Is that a framework-specific thing? + StringRef ModuleMap = this->ModuleMap ? this->ModuleMap->getName() : InFile; Please pick a different variable name rather than shadowing a member of '*this' here. + // Construct the name <ModuleName>-<hash of ModuleMapPath>.pcm which should + // be globally unique to this particular module. + llvm::APInt Code(64, llvm::hash_value(ModuleMapPath)); + SmallString<128> HashStr; + Code.toStringUnsigned(HashStr); Use base 36, like the module hash.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
