Re: Add a new spelling for module map files 'module.modulemap'

[Ben Langmuir]

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

modulemap.patch
Description: Binary data

On Mar 18, 2014, at 12:43 PM, Ben Langmuir <blangm...@apple.com> wrote: On Mar 18, 2014, at 12:05 PM, Richard Smith <rich...@metafoo.co.uk> 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 <blangm...@apple.com> wrote: On Mar 18, 2014, at 10:48 AM, Ben Langmuir <blangm...@apple.com> wrote: > > On Mar 18, 2014, at 10:19 AM, Dmitri Gribenko <griboz...@gmail.com> wrote: > >> On Tue, Mar 18, 2014 at 4:52 PM, Ben Langmuir <blangm...@apple.com> 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 <griboz...@gmail.com>*/ > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits