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

Reply via email to