[PATCH] D49466: Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:609
 static void addDebugPrefixMapArg(const Driver &D, const ArgList &Args, ArgStringList &CmdArgs) {
+  for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ)) {
+    StringRef Map = A->getValue();
----------------
I find it confusing that `-ffile_prefix_map` implies `-fdebug-prefix-map`. I'm not sure that is desirable in every case. It seems better to have a combined option that explicitly does both.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:612
+    if (Map.find('=') == StringRef::npos)
+      D.Diag(diag::err_drv_invalid_argument_to_fdebug_prefix_map) << Map;
+    else
----------------
I'd prefer the bailout style here, i.e. `if (...) { D.diag(...); continue }`


================
Comment at: lib/Driver/ToolChains/Clang.cpp:628
+/// Add a CC1 option to specify the macro file path prefix map.
+static void addMacroPrefixMapArg(const Driver &D, const ArgList &Args, ArgStringList &CmdArgs) {
+  for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ)) {
----------------
erichkeane wrote:
> See advice above.
> 
> Additionally/alternatively, I wonder if these two functions could be trivially combined.
Or at least the for loop could be refactored into a small helper function that takes the option name, output option and error as argument.


================
Comment at: lib/Lex/PPMacroExpansion.cpp:1460
+  for (const auto &Entry : MacroPrefixMap)
+    if (Path.startswith(Entry.first))
+      return (Twine(Entry.second) + Path.substr(Entry.first.size())).str();
----------------
This doesn't handle directory vs string prefix prefix correctly, does it? At the very least, it should have a test case :)


Repository:
  rC Clang

https://reviews.llvm.org/D49466