Committed as r204261. On Mar 18, 2014, at 1:29 PM, Richard Smith <[email protected]> wrote:
> 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
