LGTM On Tue, Mar 18, 2014 at 12:54 PM, Ben Langmuir <[email protected]> wrote:
> Updated patch attached with the following changes: > * add the framework root directory to DirectoryHasModuleMap to cutoff > lookups sooner > * reassign ModuleMapFileName, rather than assume we can drop the appended > characters > * clarify the note about 'module.map' in the docs using Richard's > suggested wording > > Ben > > > > On Mar 18, 2014, at 12:43 PM, Ben Langmuir <[email protected]> wrote: > > > On Mar 18, 2014, at 12:05 PM, Richard Smith <[email protected]> wrote: > > Documentation update: > > + Clang also accepts a module map file with the name ``module.map``, but > this will be deprecated in the future. When both ``module.modulemap`` and > ``module.map`` files are present, the ``module.modulemap`` file will be > used. > > "Will be deprecated in the future" doesn't make much sense to me. The only > functional meaning of "deprecated" here is "we intend to remove this some > day", so in that sense, if it'll be deprecated in the future, then it's > already deprecated now. Perhaps: > > + For compatibility with previous releases, if a module map file named > ``module.modulemap`` is not found, Clang will also search for a file named > ``module.map``. This behavior is deprecated and we plan to eventually > remove it. > > > Makes sense, I'll use that. > > > > + StringRef Filename = llvm::sys::path::filename(ModuleMapPath); > + SmallString<128> PrivateFilename(Directory->getName()); > + if (Filename == "module.map") > + llvm::sys::path::append(PrivateFilename, "module_private.map"); > + else if (Filename == "module.modulemap") > + llvm::sys::path::append(PrivateFilename, "module.private.modulemap"); > + else > + return nullptr; > > Maybe use StringSwitch? > > StringSwitch does not work very well here, because in the 'Default' case > we want to return early. > > I was imagining something like StringRef PrivateMap = StringSwitch<StringRef>(Filename) .Case("module.map", "module_private.map") .Case("module.modulemap", "module.private.map") .Default(StringRef()); if (PrivateMap.empty()) return nullptr; SmallString<128> PrivateFilename(Directory->getName()); llvm::sys::path::append(PrivateFilename, PrivateMap); return FileMgr.getFile(PrivateFilename); ... but I don't think this is any clearer or simpler than what you have now, so ... as you were :-) > + // Continue to allow module.map > + ModuleMapFileName.erase(ModuleMapFileName.begin() + ModuleMapDirNameLen, > + ModuleMapFileName.end()); > + > + llvm::sys::path::append(ModuleMapFileName, "module.map"); > + return FileMgr.getFile(ModuleMapFileName); > > This makes a slightly dubious assumption about what llvm::sys::path:append > does to the string (that it only appends at the character level). Even > though that assumption turns out to be correct (for now, at least), I'd > prefer to not rely on it here. > > > I'm not sure I agree that sys::path::append should ever be allowed to > modify the rest of the string, but I think what you've said applies equally > well to the rest of this function, so I'm happy to just reassign the string. > > Thanks, > > Ben > > > > On Tue, Mar 18, 2014 at 11:24 AM, Ben Langmuir <[email protected]>wrote: > >> >> On Mar 18, 2014, at 10:48 AM, Ben Langmuir <[email protected]> wrote: >> >> > >> > On Mar 18, 2014, at 10:19 AM, Dmitri Gribenko <[email protected]> >> wrote: >> > >> >> On Tue, Mar 18, 2014 at 4:52 PM, Ben Langmuir <[email protected]> >> wrote: >> >>> Thanks Argyrios. Richard & Dmitri: just to confirm - are you okay >> with me >> >>> committing this now, or did either of you intend to review the patch >> first? >> >> >> >> + // For frameworks, the preferred spelling is >> Modules/module.modulemap. >> >> + // Otherwise it is just module.modulemap. >> >> >> >> ... Otherwise it is just "module.map" at the framework root. >> > >> > WIll do. >> > >> >> >> >> Apart from that, two comments: >> >> >> >> 1. There is also the "module_private.map" thing. Do we want to rename >> >> that file as well? >> > >> > That is done in this patch as well - see loadModuleMapFileImpl. If we >> find 'module.modulemap' we will look for 'module.private.modulemap'. If we >> find 'module.map', we will look for 'module_private.map'. >> > >> >> >> >> 2. I don't immediately see how DirectoryHasModuleMap interacts with >> >> frameworks now. Does it contain only root framework directories or >> >> 'Module' subdirectories as well? Could you add a sub framework test >> >> to test this logic in hasModuleMap? >> > >> > Hmm, I didn't think about this. Right now only the 'Modules' >> subdirectory will be in DirectoryHasModuleMap unless we are going through >> hasModuleMap, in which case I believe they will both be there. This is >> probably safe, as we will not load the module map more than once, but it >> would be better to also add the framework directory so that we detect this >> earlier - I'll make that happen and add the test case you suggest. >> >> Actually we don't use hasModuleMap when searching for a sub framework. >> We go through hasModuleMap anytime we search a 'normal' directory. If you >> still think there's a missing test I'm open to suggestion though. >> >> Ben >> > >> > Ben >> > >> >> >> >> Dmitri >> >> >> >> -- >> >> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if >> >> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[email protected]>*/ >> > >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > [email protected] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
