<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Committed as r204261.<div><br><div><div>On Mar 18, 2014, at 1:29 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="gmail_extra"><div class="gmail_quote">LGTM</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Tue, Mar 18, 2014 at 12:54 PM, Ben Langmuir<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;">Updated patch attached with the following changes:<div>* add the framework root directory to DirectoryHasModuleMap to cutoff lookups sooner</div><div>* reassign ModuleMapFileName, rather than assume we can drop the appended characters</div><div>* clarify the note about ‘module.map’ in the docs using Richard’s suggested wording</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Ben</div><div><br></div><div></div></font></span></div><br><div style="word-wrap: break-word;"><div></div><div><br><div><div>On Mar 18, 2014, at 12:43 PM, Ben Langmuir <<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap: break-word;"><br><div><div>On Mar 18, 2014, at 12:05 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div>Documentation update:</div><div><br></div>+ 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.<br><div><br></div><div>"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:</div><div><br></div><div>+ 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.</div></div></blockquote><div><br></div><div>Makes sense, I’ll use that.</div></div><div><br><blockquote type="cite"><div dir="ltr"><div><br></div><div><br></div><div><div>+ StringRef Filename = llvm::sys::path::filename(ModuleMapPath);</div><div>+ SmallString<128> PrivateFilename(Directory->getName());</div><div>+ if (Filename == "module.map")</div><div>+ llvm::sys::path::append(PrivateFilename, "module_private.map");</div><div>+ else if (Filename == "module.modulemap")</div><div>+ llvm::sys::path::append(PrivateFilename, "module.private.modulemap");</div><div>+ else</div><div>+ return nullptr;</div></div><div><br></div><div>Maybe use StringSwitch?</div></div></blockquote><div>StringSwitch does not work very well here, because in the ‘Default’ case we want to return early.</div></div></div></blockquote></div></div></div></blockquote><div><br></div><div>I was imagining something like</div><div><br></div><div> StringRef PrivateMap = StringSwitch<StringRef>(Filename)</div><div> .Case("module.map", "module_private.map")</div><div> .Case("module.modulemap", "module.private.map")</div><div> .Default(StringRef());</div><div> if (PrivateMap.empty())</div><div> return nullptr;</div><div> SmallString<128> PrivateFilename(Directory->getName());</div><div> llvm::sys::path::append(PrivateFilename, PrivateMap);</div><div> return FileMgr.getFile(PrivateFilename);</div><div><br></div><div>... but I don't think this is any clearer or simpler than what you have now, so ... as you were :-)<br></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;"><blockquote type="cite"><div style="word-wrap: break-word;"><div><blockquote type="cite"><div dir="ltr"><div><div>+ // Continue to allow module.map</div><div>+ ModuleMapFileName.erase(ModuleMapFileName.begin() + ModuleMapDirNameLen,</div><div>+ ModuleMapFileName.end());</div><div>+</div><div>+ llvm::sys::path::append(ModuleMapFileName, "module.map");</div><div>+ return FileMgr.getFile(ModuleMapFileName);</div></div><div><br></div><div>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.</div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>Thanks,</div><div><br></div><div>Ben</div><br><blockquote type="cite"><div dir="ltr"></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Mar 18, 2014 at 11:24 AM, Ben Langmuir<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div><br>On Mar 18, 2014, at 10:48 AM, Ben Langmuir <<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>> wrote:<br><br>><br>> On Mar 18, 2014, at 10:19 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>> wrote:<br>><br>>> On Tue, Mar 18, 2014 at 4:52 PM, Ben Langmuir <<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>> wrote:<br>>>> Thanks Argyrios. Richard & Dmitri: just to confirm - are you okay with me<br>>>> committing this now, or did either of you intend to review the patch first?<br>>><br>>> + // For frameworks, the preferred spelling is Modules/module.modulemap.<br>>> + // Otherwise it is just module.modulemap.<br>>><br>>> ... Otherwise it is just "module.map" at the framework root.<br>><br>> WIll do.<br>><br>>><br>>> Apart from that, two comments:<br>>><br>>> 1. There is also the "module_private.map" thing. Do we want to rename<br>>> that file as well?<br>><br>> 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’.<br>><br>>><br>>> 2. I don't immediately see how DirectoryHasModuleMap interacts with<br>>> frameworks now. Does it contain only root framework directories or<br>>> 'Module' subdirectories as well? Could you add a sub framework test<br>>> to test this logic in hasModuleMap?<br>><br>> 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.<br><br></div>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.<br><span><font color="#888888"><br>Ben<br></font></span><div>><br>> Ben<br>><br>>><br>>> Dmitri<br>>><br>>> --<br>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>>*/<br>><br>><br></div><div>> _______________________________________________<br>> cfe-commits mailing list<br>><span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>><span class="Apple-converted-space"> </span><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br><br></div></blockquote></div><br></div></blockquote></div><br></div>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></blockquote></div></blockquote></div></div></div></blockquote></div><br></div></body></html>