[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris marked 6 inline comments as done.
boris added a comment.

I've marked as "done" items that I have resolved in my local revision (not yet uploaded) and have added one comment for further feedback.



================
Comment at: lib/Frontend/CompilerInvocation.cpp:982
+    StringRef Val = A->getValue();
+    if (Val.find('=') == StringRef::npos)
+      Opts.ExtraDeps.push_back(Val);
----------------
rsmith wrote:
> There should be some way to specify a module file that contains a `=` in its file name. Ideally, that way would be compatible with our currently-supported form of `-fmodule-name=filename`. I don't see a way to support that other than `stat`ing every value we're given and checking to see whether it exists before attempting to split on a `=`... thoughts?
> 
> One somewhat hackish alternative would be to only recognize an `=` if there is no preceding `/`. (So `-fmodule-file=foo=bar.pcm` specifies a mapping, and `-fmodule-file=./foo=bar.pcm` specifies the file `./foo=bar.pcm`.)
> 
> (FWIW, we also support module names containing `=` characters.)
A couple of thoughts:

1. Using stat() is also not without issues: I may have an unrelated file with a name that matches the whole value -- this will be fun to debug.

2. GCC seems to have given up on trying to support paths with '=' since AFAIK, none of their key=value options (e.g., -fdebug-prefix-map) support any kind of escaping. I think the implicit assumption is that if you are using paths with '=' in them, then you've asked for it.

3. The '/' idea will work but will get a bit hairier if we also want to support Windows (will need to check for both slashes).

4. Another option is to reserve empty module name as a way to escape '=': -fmodule-file==foo=bar.pcm. Not backwards compatible (but neither is ./) and looks a bit weird but is the simplest to implement.

My preference order is (2), (4), (3), (1). Let me know which way you want to go.




https://reviews.llvm.org/D35020