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

[Richard Smith]

LGTM

On Tue, Mar 18, 2014 at 12:54 PM, Ben Langmuir <blangmuir at apple.com> 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 <blangmuir at apple.com> wrote:
>
>
> On Mar 18, 2014, at 12:05 PM, Richard Smith <richard at 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.
>
>
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 <blangmuir at apple.com>wrote:
>
>>
>> On Mar 18, 2014, at 10:48 AM, Ben Langmuir <blangmuir at apple.com> wrote:
>>
>> >
>> > On Mar 18, 2014, at 10:19 AM, Dmitri Gribenko <gribozavr at gmail.com>
>> wrote:
>> >
>> >> On Tue, Mar 18, 2014 at 4:52 PM, Ben Langmuir <blangmuir at 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 <gribozavr at gmail.com>*/
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140318/95b8461e/attachment.html>