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. > > > + // 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
